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

Create tables with fully qualified name for custom resources #228

Merged

Conversation

afarid
Copy link
Contributor

@afarid afarid commented May 30, 2024

Changes:
Creating tables for custom resources with fully qualified name even there is no collisions. This will help to query exact CR if there are multiple resources with the same singular name.

@misraved misraved requested a review from ParthaI May 31, 2024 09:02
Comment on lines +127 to +128
ctx = context.WithValue(ctx, contextKey("TableName"), "kubernetes_"+crd.Spec.Names.Singular+"_"+re.ReplaceAllString(crd.Spec.Group, "_"))
tables["kubernetes_"+crd.Spec.Names.Singular+"_"+re.ReplaceAllString(crd.Spec.Group, "_")] = tableKubernetesCustomResource(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hello @afarid, I believe the code's behavior will remain the same as before. We are not changing the table naming, just removing the else block. With the previous code, the else block always executes when the if condition is not met.

Attaching the Slack Conversation ref: https://turbot-community.slack.com/archives/C01UECB59A7/p1717092252633259

E.g:
1st one:
k8s Group: s3.aws.upbound.io
Kind: Buckets
2nd one:
k8s Group: storage.gcp.upbound.io
Kind: Buckets

In both cases, I believe the current and previous code will construct the table names as buckets.s3.aws.upbound.io and buckets.storage.gcp.upbound.io.

Are there any special cases where the else block code does not execute?

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ParthaI No, I don't see any special cases the else block code does not execute. so we can consistent naming for all CRDs

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for your input, @afarid. Considering there are no special cases where the else block doesn't execute.

I'm thinking of the necessity of the code changes. Could you please provide further insight into how these changes improve upon the existing code and whether the current implementation adequately resolves the issue(I believe the existing code should have consistent naming for all CRDs)?

Copy link
Contributor Author

@afarid afarid Jun 5, 2024

Choose a reason for hiding this comment

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

@ParthaI Sorry for the confusion.
with existing code, There is only one case when else statement is not executed. for example if there is custom custom resource name buckets.s3.aws.upbound.io, one table will be created withe name kubernetes_bucket. but I need to have full name bucket_s3_aws_upbound_io. This is the only corner case that I need to have.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, @afarid, I believe the else block will always be executed if the if condition is not met. Here's an illustration:

For example:

  1. First Scenario:

    • k8s Group: s3.aws.upbound.io
    • Kind: Buckets
  2. Second Scenario:

    • k8s Group: storage.gcp.upbound.io
    • Kind: Buckets
  • In both cases, the kind for two different CRDs is the same.
  • During the first loop (k8s Group: s3.aws.upbound.io), it checks if there's an existing table with the CRD's singular name (if tables["kubernetes_"+crd.Spec.Names.Singular] == nil {):
    • If no table exists, it creates a table with the singular name. e.g: kubernetes_bucket.
    • If a table already exists, it creates the table with a combination of the singular name and the group, such as kubernetes_s3_aws_upbound_io.
  • In the second loop (k8s Group: storage.gcp.upbound.io), based on the previous outcome:
    • If the first loop created the table kubernetes_bucket,
    • The table for the second loop will be created as kubernetes_bucket_storage_gcp_upbound_io.

Please let me know if there’s something I might be overlooking.

Is there any scenario where the table configuration is being overridden? I believe this shouldn't happen since we first perform a check before configuring at this line: if tables["kubernetes_" + crd.Spec.Names.Singular] == nil {.

Could you please outline the steps needed to replicate the issue in my local environment? So that I can try to understand the edge cases.

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, gotcha! @afarid, the table is functioning as expected, but we have a concern that the table name is a bit confusing, right?

yes, I found that table and I could query the correct data.
The problem is that I need to query by full table name kubernetes_bucket_s3_aws_upbound_io as I found that sometime kubernetes_buckets points to kubernetes_bucket_s3_aws_upbound_io and sometimes it points to buckets.storage.gcp.upbound.io

I got confused by the conversation in the Slack thread.

What tables will be added if we remove the if clause logic? Will we still be able to see the kubernetes_bucket table?
With the current changes, will the kubernetes_bucket and kubernetes_bucket_s3_aws_upbound_io tables have the same data since they both refer to a single CRD?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly. We will still see kubernetes_bucket tables, and we will have two tables pointing to the same data. One with a short name and the other with a fully qualified name. The change is backward compatible, so users can still use kubernetes_.

You can try adding two different CRDs with the same kind but different group versions to see the result.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I will try to reproduce it with different edge cases and let you know.

We will still see the kubernetes_bucket table.

This is without the if condition logic, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ParthaI Yes, kubernetes_<singular_name> will still exist in both cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello @afarid, I was doing some investigations regarding the table schema along with the table naming convention for the CRDs. Here is the observation from my end. Please have a look. Thanks!

I have the CRDs as mentioned:

NAME CREATED AT
buckets.s3.aws.upbound.io 2024-06-06T04:21:51Z
buckets.storage.gcp.upbound.io 2024-06-06T04:21:40Z
certificates.cert-manager.io 2023-12-18T05:15:55Z
mycustomresources.example.com 2023-12-14T15:47:28Z

Case 1: With the Existing Code

if tables["kubernetes_"+crd.Spec.Names.Singular] == nil {
	plugin.Logger(ctx).Warn("Table name from if => ", "kubernetes_"+crd.Spec.Names.Singular)
	ctx = context.WithValue(ctx, contextKey("TableName"), "kubernetes_"+crd.Spec.Names.Singular)
	tables["kubernetes_"+crd.Spec.Names.Singular] = tableKubernetesCustomResource(ctx)
} else {
	plugin.Logger(ctx).Warn("Table name from else => ", "kubernetes_"+crd.Spec.Names.Singular+"_"+re.ReplaceAllString(crd.Spec.Group, "_"))
	ctx = context.WithValue(ctx, contextKey("TableName"), "kubernetes_"+crd.Spec.Names.Singular+"_"+re.ReplaceAllString(crd.Spec.Group, "_"))
	tables["kubernetes_"+crd.Spec.Names.Singular+"_"+re.ReplaceAllString(crd.Spec.Group, "_")] = tableKubernetesCustomResource(ctx)
}

Table Names:

  • From if => kubernetes_bucket
  • From else => kubernetes_bucket_storage_gcp_upbound_io
  • From if => kubernetes_certificate
  • From if => kubernetes_mycustomresource

Case 2: With the Code Changes in this PR(We are having duplicate tables)

if tables["kubernetes_"+crd.Spec.Names.Singular] == nil {
	plugin.Logger(ctx).Warn("Table name from if => ", "kubernetes_"+crd.Spec.Names.Singular)
	ctx = context.WithValue(ctx, contextKey("TableName"), "kubernetes_"+crd.Spec.Names.Singular)
	tables["kubernetes_"+crd.Spec.Names.Singular] = tableKubernetesCustomResource(ctx)
}
plugin.Logger(ctx).Warn("Table name from else => ", "kubernetes_"+crd.Spec.Names.Singular+"_"+re.ReplaceAllString(crd.Spec.Group, "_"))
ctx = context.WithValue(ctx, contextKey("TableName"), "kubernetes_"+crd.Spec.Names.Singular+"_"+re.ReplaceAllString(crd.Spec.Group, "_"))
tables["kubernetes_"+crd.Spec.Names.Singular+"_"+re.ReplaceAllString(crd.Spec.Group, "_")] = tableKubernetesCustomResource(ctx)

Table Names:

  • From if => kubernetes_bucket
  • From else => kubernetes_bucket_s3_aws_upbound_io
  • From else => kubernetes_bucket_storage_gcp_upbound_io
  • From if => kubernetes_certificate
  • From else => kubernetes_certificate_cert_manager_io
  • From if => kubernetes_mycustomresource
  • From else => kubernetes_mycustomresource_example_com

Case 3: Removing the if Condition and Creating Tables for Active Versions Only

if activeVersion != nil && activeVersion.Name != "" {
	plugin.Logger(ctx).Warn("Table name from if with active version => ", "kubernetes_"+crd.Spec.Names.Singular+"_"+re.ReplaceAllString(crd.Spec.Group, "_")+"_"+activeVersion.Name)
	ctx = context.WithValue(ctx, contextKey("TableName"), "kubernetes_"+crd.Spec.Names.Singular+re.ReplaceAllString(crd.Spec.Group, "_"))
	tables["kubernetes_"+crd.Spec.Names.Singular+"_"+re.ReplaceAllString(crd.Spec.Group, "_")+"_"+activeVersion.Name] = tableKubernetesCustomResource(ctx)
} else {
	plugin.Logger(ctx).Warn("Table name from else => ", "kubernetes_"+crd.Spec.Names.Singular+"_"+re.ReplaceAllString(crd.Spec.Group, "_"))
	ctx = context.WithValue(ctx, contextKey("TableName"), "kubernetes_"+crd.Spec.Names.Singular+"_"+re.ReplaceAllString(crd.Spec.Group, "_"))
	tables["kubernetes_"+crd.Spec.Names.Singular+"_"+re.ReplaceAllString(crd.Spec.Group, "_")] = tableKubernetesCustomResource(ctx)
}

Table Names:

  • From if with active version => kubernetes_bucket_s3_aws_upbound_io_v1
  • From if with active version => kubernetes_bucket_storage_gcp_upbound_io_v1
  • From if with active version => kubernetes_certificate_cert_manager_io_v1
  • From if with active version => kubernetes_mycustomresource_example_com_v1

Note: If there are multiple active versions, the table will pick up the latest version's schema only.

Case 4: Creating the Table Schema with All Versions, Regardless of Active Status

Table Names:

  • Table created with name: kubernetes_bucket_s3_aws_upbound_io_v1
  • Table created with name: kubernetes_bucket_s3_aws_upbound_io_v2
  • Table created with name: kubernetes_bucket_storage_gcp_upbound_io_v1
  • Table created with name: kubernetes_certificate_cert_manager_io_v1
  • Table created with name: kubernetes_mycustomresource_example_com_v1

Note: From the current main branch, I identified a scenario where the plugin throws an error if a CRD has no active versions.

I have pushed the change to the branch update-table-naming-convention, which includes the change for Case 4.

Could you please give it a try in the PR branch and share your feedback with us?
Steps to try out the code changes in local:

Thanks!

@afarid afarid requested a review from ParthaI June 5, 2024 08:24
@ParthaI
Copy link
Contributor

ParthaI commented Jul 17, 2024

Hi @misraved, could you please review and proceed with releasing this PR?

@misraved misraved merged commit ca0b2b2 into turbot:main Jul 26, 2024
1 check passed
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