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

feat: Add support for cortex search service #2860

Merged
merged 9 commits into from
Jul 1, 2024

Conversation

chris-codaio
Copy link
Contributor

@chris-codaio chris-codaio commented Jun 7, 2024

Adds resource & datasource for Cortex search service objects.

Test Plan

  • unit tests
  • acceptance tests
  • integration tests
  • real usage in my private repo/account

PTAL @sfc-gh-mloring

Currently blocked on what looks like a bug in the SQL create flow where the following occurs.

CREATE CORTEX SEARCH SERVICE CODA.CHRIS.CHRIS_TF_TEST
  ON TEXT_COL
  WAREHOUSE = "TEST"
  TARGET_LAG = '1 hour'
  COMMENT = 'Chris Terraform Test Search Service'
  AS
    SELECT USER_ID, DOC_ID, TEXT_COL FROM CODA.CHRIS.CHRIS_TF_TEST;

Cannot perform operation. This session does not have a current database. Call 'USE DATABASE', or use a qualified name.

@sfc-gh-mloring
Copy link

@chris-codaio Thank you so much for putting this together! I'm going to take a look at the compilation error you hit and then circle back to this PR. @sfc-gh-asawicki would you be able to help find the right reviewer for this?

@sfc-gh-jcieslak sfc-gh-jcieslak self-requested a review June 10, 2024 04:39
@sfc-gh-asawicki sfc-gh-asawicki self-requested a review June 10, 2024 11:30
@sfc-gh-asawicki
Copy link
Collaborator

Hey @chris-codaio @sfc-gh-mloring. Thanks for the contribution!

We will review it in a couple of days (as described in https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/CONTRIBUTING.md#requesting-the-review). Before that please make sure that the PR follows all the requirements described in https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/CONTRIBUTING.md#contributing. (one thing I noticed from the first glance is that the SDK part is created manually and not using the existing generator: https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/CONTRIBUTING.md#introducing-a-new-part-of-the-sdk).

@chris-codaio
Copy link
Contributor Author

Hey @chris-codaio @sfc-gh-mloring. Thanks for the contribution!

We will review it in a couple of days (as described in https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/CONTRIBUTING.md#requesting-the-review). Before that please make sure that the PR follows all the requirements described in https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/CONTRIBUTING.md#contributing. (one thing I noticed from the first glance is that the SDK part is created manually and not using the existing generator: https://github.com/Snowflake-Labs/terraform-provider-snowflake/blob/main/CONTRIBUTING.md#introducing-a-new-part-of-the-sdk).

Hello @sfc-gh-asawicki ,

I initially skipped the generator given the lack of documentation linking how to setup various field structures from documentation to its internal builder. I just slogged my way through it - hope that looks reasonable?

@sfc-gh-jmichalak
Copy link
Collaborator

Hi @chris-codaio, we will review this PR once this is available in our testing environment, can't promise any specific date.

@sfc-gh-mloring
Copy link

Your test accounts should be enabled now.

@sfc-gh-asawicki
Copy link
Collaborator

Hey @chris-codaio @sfc-gh-mloring, I just wanted to let you know that we had to postpone the tests and review because of internal issues that the team had to tend to. Currently, I estimate we will be able to spend time on it on Friday.

We are sorry for the delay.

The closet release is expected at the end of next week or later, so it is still possible that this PR will be a part of it.

@sfc-gh-asawicki
Copy link
Collaborator

sfc-gh-asawicki commented Jun 21, 2024

Hey @chris-codaio @sfc-gh-mloring. Cross-posting the questions asked internally for transparency. We would like to have the answers before proceeding with this PR because the answers will affect the design of the resource.

  1. CREATE
    1. Are ATTRIBUTES required or optional? From the docs they seem required, but I was able to create without.
    2. How can we get the <query> after creation (SHOW/DESCRIBE do not return it)?
    3. How can we get the ATTRIBUTES after creation (SHOW/DESCRIBE do not return them)?
    4. <query> is missing in the required parameters section.
    5. Why <query> in the examples is wrapped in parentheses if there are no parentheses in the syntax? (I checked, and it works without them too)
  2. ALTER
    1. Will UNSET comment be supported?
    2. Will setting/unsetting/adding/removing <attributes> be supported?
    3. Will setting <query> be supported?
    4. Will setting <search_column> be supported?
    5. Will rename be supported?
  3. SHOW
    1. Are any more columns planned?
    2. Target lag is listed in the docs but it is not returned.
  4. DESCRIBE
    1. What is the included columns in the output? How is it connected with ATTRIBUTES used in creation? It was empty when I created with ATTRIBUTES, and it was filled when I did not specify them.
    2. Why comment is listed both on show and describe?
    3. Docs are incomplete (not all columns are listed and explained).
  5. DROP
    1. IF EXISTS is missing in the docs, but it works.

Copy link
Collaborator

@sfc-gh-jcieslak sfc-gh-jcieslak left a comment

Choose a reason for hiding this comment

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

Hey @chris-codaio, Thanks again for the contribution. I've added some comments, but some of them are affected by the answer to the @sfc-gh-asawicki question/list of things we have to know. You can skip those for now, but the rest should be valid comments.

pkg/sdk/cortex_search_services_def.go Outdated Show resolved Hide resolved
pkg/sdk/cortex_search_services_def.go Outdated Show resolved Hide resolved
pkg/sdk/cortex_search_services_def.go Outdated Show resolved Hide resolved
pkg/sdk/cortex_search_services_def.go Show resolved Hide resolved
pkg/sdk/cortex_search_services_def.go Show resolved Hide resolved
pkg/resources/cortex_search_service.go Outdated Show resolved Hide resolved
pkg/resources/cortex_search_service.go Show resolved Hide resolved
pkg/resources/cortex_search_service.go Outdated Show resolved Hide resolved
pkg/resources/cortex_search_service.go Outdated Show resolved Hide resolved
@sfc-gh-mloring
Copy link

Thank you for taking such a detailed look and for suggesting so many improvements to our docs. I'll work with our doc writer to get these updated!

Hey @chris-codaio @sfc-gh-mloring. Cross-posting the questions asked internally for transparency. We would like to have the answers before proceeding with this PR because the answers will affect the design of the resource.

  1. CREATE

    1. Are ATTRIBUTES required or optional? From the docs they seem required, but I was able to create without.

Attributes are optional, we'll update the docs.

  1. How can we get the after creation (SHOW/DESCRIBE do not return it)?

Looks like a word is missing.

  1. How can we get the ATTRIBUTES after creation (SHOW/DESCRIBE do not return them)?

These are included in describe output in a recent change that is still waiting to roll out to production.

  1. is missing in the required parameters section.

Looks like a word is missing here too.

  1. Why in the examples is wrapped in parentheses if there are no parentheses in the syntax? (I checked, and it works without them too)

Yep, these are optional. We'll get them removed from the docs.

  1. ALTER

    1. Will UNSET comment be supported?
    2. Will setting/unsetting/adding/removing be supported?
    3. Will setting be supported?
    4. Will setting <search_column> be supported?
    5. Will rename be supported?

We don't have near term plans for these ALTER capabilities.

  1. SHOW

    1. Are any more columns planned?

Yes. As we move from private preview to public preview, we expect to include more information in both show and describe.

  1. Target lag is listed in the docs but it is not returned.

The change to add this to describe has already landed. The changes to show are in progress.

  1. DESCRIBE

    1. What is the included columns in the output? How is it connected with ATTRIBUTES used in creation? It was empty when I created with ATTRIBUTES, and it was filled when I did not specify them.

Attributes are the set of columns that you want to perform filtering on at search time. Any columns specified in the projection list of the source query that are not named as "attributes" are included columns. These columns cannot be searched over or filtered on but can be included in search results if desired.

  1. Why comment is listed both on show and describe?
  2. Docs are incomplete (not all columns are listed and explained).

We're still in private preview and both show and describe output is in flux. We'll get the docs updated once we've decided on the final structure. Is terraform dependent on the specific output format of show/describe or are these questions more general?

  1. DROP

    1. IF EXISTS is missing in the docs, but it works.

Thanks for catching. We'll update the docs.

@sfc-gh-asawicki
Copy link
Collaborator

  1. How can we get the after creation (SHOW/DESCRIBE do not return it)?

Looks like a word is missing.

Ech, sorry GH markdown thought that <query> is a HTML attribute. I corrected the message (it was correct on slack, though).

  1. How can we get the ATTRIBUTES after creation (SHOW/DESCRIBE do not return them)?

These are included in describe output in a recent change that is still waiting to roll out to production.

Okay, this affects the potential implementation in the provider.

  1. is missing in the required parameters section.

Looks like a word is missing here too.

Same as above: <query>.

  1. ALTER

    1. Will UNSET comment be supported?
    2. Will setting/unsetting/adding/removing <attributes> be supported?
    3. Will setting <query> be supported?
    4. Will setting <search_column> be supported?
    5. Will rename be supported?

We don't have near term plans for these ALTER capabilities.

They are really helpful for a proper terraform provider implementation. Without them we have to drop and recreate the resource with almost every change.

  1. SHOW

    1. Are any more columns planned?

Yes. As we move from private preview to public preview, we expect to include more information in both show and describe.

From the provider point of view: every parameter used in create should be returned in SHOW or DESCRIBE; this is how we can discover the changes both in config and the external ones (e.g. manual in worksheet) from the provider side.

  1. Why comment is listed both on show and describe?
  2. Docs are incomplete (not all columns are listed and explained).

We're still in private preview and both show and describe output is in flux. We'll get the docs updated once we've decided on the final structure. Is terraform dependent on the specific output format of show/describe or are these questions more general?

Terraform is highly dependent on the information that can be retrieved from SHOW/DESC (as in one of the points above).

@chris-codaio
Copy link
Contributor Author

@sfc-gh-asawicki @sfc-gh-mloring - I've made a bunch of updates to address the outstanding feedback. A couple of remaining questions for you.

Copy link
Collaborator

@sfc-gh-asawicki sfc-gh-asawicki left a comment

Choose a reason for hiding this comment

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

I have checked the change, and I like it. Thanks for all the changes after @sfc-gh-jcieslak reviews. I have left a few small comments/answers but I would like to proceed and merge this change.

We have some stuff we have to add before it gets released so we can also cover a few more changes/corrections. Just FYI for you @chris-codaio, for future reference (we still update our contribution guidelines with the conventions that our community is not aware of and I will add the following topics):

  • import instructions (they have to be added as a separate file in the examples directory so they can be automatically added to the generated docs)
  • examples (same as above)
  • test for added privileges (because the object is new, we have to test its correctness with at least an integration test on the SDK level before we can release it)
  • in the SDK tests we tend to keep them all inside one test wrapping function (not a function dedicated for each group of requests - this helps with shorter set ups)
  • formatting and docs regeneration (I noticed that make pre-push was not run after the latest changes)
  • [this one is a more of a nit-pick] - instead of checking the created date for the changes on the objects, we can check the plan for the resource (planchecks.ExpectResourceAction)
  • (the ones from my comments): missing forceNew, grants to be removed

pkg/resources/cortex_search_service.go Show resolved Hide resolved
pkg/resources/cortex_search_service.go Show resolved Hide resolved
https://pkg.go.dev/time
note: format may depend on what the account parameter for TIMESTAMP_OUTPUT_FORMAT is set to. Perhaps we should return this as a string rather than a time.Time?
*/
record["created_on"] = dt.CreatedOn.Format("2006-01-02T16:04:05.000 -0700")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, let's do String() here pls.

Columns: []string{"product_id", "product_name"},
}
opts.Comment = String("comment")
assertOptsValidAndSQLEquals(t, opts, `CREATE CORTEX SEARCH SERVICE IF NOT EXISTS %s ON searchable_text ATTRIBUTES product_id, product_name WAREHOUSE = "warehouse_name" TARGET_LAG = '1 minutes' COMMENT = 'comment' AS SELECT product_id, product_name, searchable_text FROM staging_table`, id.FullyQualifiedName())
Copy link
Collaborator

Choose a reason for hiding this comment

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

From the answer we get from @sfc-gh-mloring I would say it's fine:

Yep, these are optional. We'll get them removed from the docs.

@@ -204,6 +206,7 @@ const (
PluralObjectTypeShares PluralObjectType = "SHARES"
PluralObjectTypeTables PluralObjectType = "TABLES"
PluralObjectTypeDynamicTables PluralObjectType = "DYNAMIC TABLES"
PluralObjectTypeCortexSearchServices PluralObjectType = "CORTEX SEARCH SERVICES"
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1, let's remvoe them.

Also, we should have a test if granting the privilege works for the singular object

pkg/sdk/privileges.go Show resolved Hide resolved
@sfc-gh-asawicki
Copy link
Collaborator

/ok-to-test sha=4c0f856994da281a20b3f2fb29e6a02477d9291e

Copy link

github-actions bot commented Jul 1, 2024

Integration tests failure for

@sfc-gh-asawicki sfc-gh-asawicki changed the title Initial pass over creation of Cortex search service support feat: Add support for cortex search service Jul 1, 2024
Copy link

github-actions bot commented Jul 1, 2024

Integration tests failure for 4c0f856994da281a20b3f2fb29e6a02477d9291e

1 similar comment
Copy link

github-actions bot commented Jul 1, 2024

Integration tests failure for 4c0f856994da281a20b3f2fb29e6a02477d9291e

@sfc-gh-asawicki sfc-gh-asawicki merged commit 43aa89f into Snowflake-Labs:main Jul 1, 2024
6 of 9 checks passed
sfc-gh-jcieslak pushed a commit that referenced this pull request Jul 10, 2024
🤖 I have created a release *beep* *boop*
---


##
[0.93.0](v0.92.0...v0.93.0)
(2024-07-10)


### 🎉 **What's new:**

* Add OAUTH integration for custom clients
([#2908](#2908))
([d9b557f](d9b557f))
* Add oauth integration for partner applications
([#2912](#2912))
([91788e5](91788e5))
* Add support for cortex search service
([#2860](#2860))
([43aa89f](43aa89f))
* API Authentication integration v1 readiness
([#2898](#2898))
([91931da](91931da))
* External Oauth integration v1 readiness
([#2907](#2907))
([ed237c3](ed237c3))
* Generate show outputs with mappers
([#2886](#2886))
([1cada88](1cada88))
* Introduce security integrations datasource
([#2892](#2892))
([7f6c657](7f6c657))
* SAML2 integration v1 readiness
([#2868](#2868))
([d0c136d](d0c136d))
* SCIM integration v1 readiness
([#2846](#2846))
([269df6b](269df6b))
* Security integrations datasource v1 readiness
([#2913](#2913))
([d10474a](d10474a))
* standard database v1 readiness
([#2842](#2842))
([3c11953](3c11953))
* Warehouse redesign final touches
([#2900](#2900))
([0eab636](0eab636))
* Warehouse redesign part1
([#2864](#2864))
([6664457](6664457))
* Warehouse redesign part2
([#2887](#2887))
([1aaf417](1aaf417))
* Warehouse redesign part3
([#2890](#2890))
([873a1ed](873a1ed))
* Warehouse redesign part4
([#2893](#2893))
([d525fd9](d525fd9))


### 🔧 **Misc**

* Add documentation on unset and defaults
([#2882](#2882))
([85a7836](85a7836))
* apply minor database changes
([#2872](#2872))
([6ccac59](6ccac59))
* Apply new resource conventions to scim integration
([#2891](#2891))
([e11e608](e11e608))
* Improve generator template organization
([#2820](#2820))
([5035e2f](5035e2f))
* Nuke stale objects
([#2869](#2869))
([9c4a117](9c4a117))
* Show a possible solution for
[#2877](https://github.com/Snowflake-Labs/terraform-provider-snowflake/issues/2877)
([#2878](#2878))
([6fb437b](6fb437b))
* Validations cleanup and old grants removal
([#2884](#2884))
([05b7eee](05b7eee))


### 🐛 **Bug fixes:**

* Add disclaimers and fix tests
([#2905](#2905))
([1deaedc](1deaedc))
* Fix cortex search service
([#2904](#2904))
([763d06c](763d06c))
* use suppressQuoting to fix stage file_format permadiff
([#2885](#2885))
([fd70f6e](fd70f6e))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: snowflake-release-please[bot] <105954990+snowflake-release-please[bot]@users.noreply.github.com>
@sfc-gh-asawicki
Copy link
Collaborator

Hey @sfc-gh-mloring @chris-codaio. We have released v0.93.0 yesterday containing cortex search service resource and datasource.

Thanks for the contribution one more time @chris-codaio!

@chris-codaio chris-codaio deleted the add-cortex-search-service branch July 22, 2024 17:07
@chris-codaio
Copy link
Contributor Author

Just back from vacation and looking at this again. Looks like we still can't use this end to end given the following query doesn't work, seemingly necessary to use the snowflake_grant_privileges_to_account_role resource to set a grant on the search service.

SHOW GRANTS ON CORTEX SEARCH SERVICE <name>.

@sfc-gh-mloring - is that something you can help with?

@chris-codaio
Copy link
Contributor Author

Also, looks like we have a bug somewhere in trying to pass in that attribute set(string).

Stack trace from the terraform-provider-snowflake_v0.93.0 plugin:

panic: interface conversion: interface {} is *schema.Set, not []string

goroutine 42 [running]:
github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/resources.CreateCortexSearchService({0x103d264e0, 0x140002ead20}, 0x0?, {0x103926260?, 0x1400063c6b0?})
	github.com/Snowflake-Labs/terraform-provider-snowflake/pkg/resources/cortex_search_service.go:150 +0x6c8
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*Resource).create(0x14000651880, {0x103d26438, 0x14000998ba0}, 0xd?, {0x103926260, 0x1400063c6b0})
	github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema/resource.go:778 +0xe8
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*Resource).Apply(0x14000651880, {0x103d26438, 0x14000998ba0}, 0x140002da410, 0x140001c1800, {0x103926260, 0x1400063c6b0})
	github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema/resource.go:909 +0x86c
github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*GRPCProviderServer).ApplyResourceChange(0x14000c48300, {0x103d26438?, 0x140009989f0?}, 0x14000f94cd0)
	github.com/hashicorp/terraform-plugin-sdk/[email protected]/helper/schema/grpc_provider.go:1078 +0xb08
github.com/hashicorp/terraform-plugin-mux/tf5to6server.v5tov6Server.ApplyResourceChange({{0x103d3c918?, 0x14000c48300?}}, {0x103d26438, 0x140009989f0}, 0x0?)
	github.com/hashicorp/[email protected]/tf5to6server/tf5to6server.go:47 +0x58
github.com/hashicorp/terraform-plugin-mux/tf6muxserver.(*muxServer).ApplyResourceChange(0x103d26470?, {0x103d26438?, 0x140009986f0?}, 0x14000f94c80)
	github.com/hashicorp/[email protected]/tf6muxserver/mux_server_ApplyResourceChange.go:36 +0x184
github.com/hashicorp/terraform-plugin-go/tfprotov6/tf6server.(*server).ApplyResourceChange(0x14000c541e0, {0x103d26438?, 0x140002dfec0?}, 0x140002ea380)
	github.com/hashicorp/[email protected]/tfprotov6/tf6server/server.go:846 +0x2b0
github.com/hashicorp/terraform-plugin-go/tfprotov6/internal/tfplugin6._Provider_ApplyResourceChange_Handler({0x103c8c7a0?, 0x14000c541e0}, {0x103d26438, 0x140002dfec0}, 0x140001c1300, 0x0)
	github.com/hashicorp/[email protected]/tfprotov6/internal/tfplugin6/tfplugin6_grpc.pb.go:518 +0x164
google.golang.org/grpc.(*Server).processUnaryRPC(0x14000ac8a00, {0x103d26438, 0x140002dfe30}, {0x103d37e60, 0x1400001a300}, 0x140002efc20, 0x14000ceaa80, 0x104b3cc78, 0x0)
	google.golang.org/[email protected]/server.go:1369 +0xba0
google.golang.org/grpc.(*Server).handleStream(0x14000ac8a00, {0x103d37e60, 0x1400001a300}, 0x140002efc20)
	google.golang.org/[email protected]/server.go:1780 +0xc80
google.golang.org/grpc.(*Server).serveStreams.func2.1()
	google.golang.org/[email protected]/server.go:1019 +0x8c
created by google.golang.org/grpc.(*Server).serveStreams.func2 in goroutine 50
	google.golang.org/[email protected]/server.go:1030 +0x150

Error: The terraform-provider-snowflake_v0.93.0 plugin crashed!

This is always indicative of a bug within the plugin. It would be immensely
helpful if you could report the crash with the plugin's maintainers so that it
can be fixed. The output above should help diagnose the issue.

@sfc-gh-mloring
Copy link

Just back from vacation and looking at this again. Looks like we still can't use this end to end given the following query doesn't work, seemingly necessary to use the snowflake_grant_privileges_to_account_role resource to set a grant on the search service.

SHOW GRANTS ON CORTEX SEARCH SERVICE <name>.

@sfc-gh-mloring - is that something you can help with?

Fix is already landed and rolling out with 8.27 in the next few days.

sfc-gh-asawicki pushed a commit that referenced this pull request Jul 23, 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.

5 participants