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

bug: resolving refs/v1beta1 before reference resource is ready #3619

Open
acpana opened this issue Feb 6, 2025 · 1 comment
Open

bug: resolving refs/v1beta1 before reference resource is ready #3619

acpana opened this issue Feb 6, 2025 · 1 comment

Comments

@acpana
Copy link
Collaborator

acpana commented Feb 6, 2025

Most (if not all) references under refs/v1beta1 contain a logical issue where we can attempt to resolve a reference without knowing that the k8s-resource for that resource is actually READY to be resolved. Consider:

func ResolveBigQueryDataset(ctx context.Context, reader client.Reader, src client.Object, ref *BigQueryDatasetRef) (*BigQueryDataset, error) {
if ref == nil {
return nil, nil
}
if ref.Name == "" && ref.External == "" {
return nil, fmt.Errorf("must specify either name or external on BigQueryDatasetRef")
}
if ref.Name != "" && ref.External != "" {
return nil, fmt.Errorf("cannot specify both name and external on BigQueryDatasetRef")
}
// External is provided.
if ref.External != "" {
// External should be in the `projects/[project_id]/datasets/[dataset_id]` format.
tokens := strings.Split(ref.External, "/")
if len(tokens) == 4 && tokens[0] == "projects" && tokens[2] == "datasets" {
return &BigQueryDataset{
projectID: tokens[1],
datasetID: tokens[3],
}, nil
}
return nil, fmt.Errorf("format of BigQueryDatasetRef external=%q was not known (use projects/[project_id]/datasets/[dataset_id])", ref.External)
}
// Fetch BigQueryDataset object to construct the external form.
dataset := &unstructured.Unstructured{}
dataset.SetGroupVersionKind(schema.GroupVersionKind{
Group: "bigquery.cnrm.cloud.google.com",
Version: "v1beta1",
Kind: "BigQueryDataset",
})
nn := types.NamespacedName{
Namespace: ref.Namespace,
Name: ref.Name,
}
if nn.Namespace == "" {
nn.Namespace = src.GetNamespace()
}
if err := reader.Get(ctx, nn, dataset); err != nil {
if apierrors.IsNotFound(err) {
return nil, fmt.Errorf("referenced BigQueryDataset %v not found", nn)
}
return nil, fmt.Errorf("error reading referenced BigQueryDataset %v: %w", nn, err)
}
projectID, err := ResolveProjectID(ctx, reader, dataset)
if err != nil {
return nil, err
}
datasetID, err := GetResourceID(dataset)
if err != nil {
return nil, err
}
return &BigQueryDataset{
projectID: projectID,
datasetID: datasetID,
}, nil
}

This can basically lead to extra calls to GCP when the underlying dependency is not actually ready.

This issue has come up in #3422 (comment) . We should be weary of this too when moving on to the new reference style and treating migrations (e.g.from TF), especially on acquisition on existing resource where status.ExternalRef may not be populated.

@jingyih
Copy link
Collaborator

jingyih commented Feb 6, 2025

I had a brief chat with @yuwenma right after our discussion on this. "Not waiting for the reference to be READY" may be working as intended? I'll defer to @yuwenma on this.

@acpana acpana changed the title bug: resolving refs/v1bbeta1 before reference resource is ready bug: resolving refs/v1beta1 before reference resource is ready Feb 6, 2025
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

No branches or pull requests

2 participants