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

Observe with Terraform Import when Observe Only #168

Merged
merged 5 commits into from
Apr 13, 2023
Merged

Observe with Terraform Import when Observe Only #168

merged 5 commits into from
Apr 13, 2023

Conversation

turkenh
Copy link
Member

@turkenh turkenh commented Mar 2, 2023

Description of your changes

This PR add support Observe Only management policy to the upjet common controller.

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

By following the procedure here and observing some resources, AWS VPC, AWS EKS Cluster ...

@turkenh turkenh mentioned this pull request Mar 2, 2023
3 tasks
@turkenh turkenh requested a review from lsviben March 2, 2023 09:05
@turkenh turkenh marked this pull request as ready for review March 2, 2023 10:37
Copy link
Member

@sergenyalcin sergenyalcin left a comment

Choose a reason for hiding this comment

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

Thanks @turkenh I asked a question

pkg/controller/external.go Outdated Show resolved Hide resolved
@turkenh
Copy link
Member Author

turkenh commented Apr 10, 2023

Rebased on latest main.

Copy link
Collaborator

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

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

Thank you @turkenh, left some comments for you to consider.

pkg/controller/external.go Show resolved Hide resolved
pkg/terraform/workspace.go Outdated Show resolved Hide resolved
pkg/terraform/workspace.go Outdated Show resolved Hide resolved
pkg/terraform/workspace.go Show resolved Hide resolved
// way we can do it for now. Terraform import does not return a proper exit code for this case or
// does not support -json flag to parse the returning error in a better way.
// https://github.com/hashicorp/terraform/blob/93f9cff99ffbb8d536b276a1be40a2c45ca4a67f/internal/terraform/node_resource_import.go#L235
if strings.Contains(string(out), "Cannot import non-existent remote object") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to reliably use the Terraform state to deduce a non-existent external resource in this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably, but I didn't feel good about reading/parsing terraform state after getting an error to my import command.

// internal/features. This is a reasonable assumption for now
// but we may want to make it configurable in the future.
featuresPkgPath := fmt.Sprintf("%s/internal/features", pc.ModulePath)
ctrlPkgPath, err := ctrlGen.Generate(resources[name], versionGen.Package().Path(), featuresPkgPath)
Copy link
Collaborator

@ulucinar ulucinar Apr 11, 2023

Choose a reason for hiding this comment

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

If I understand this correctly, we will also need the upjet-based provider to supply an EnableAlphaManagementPolicies feature flag in this internal/features package looking at the changes in the controller template pkg/pipeline/templates/controller.go.tmpl.

If this is correct, we will also need a corresponding change in the template repository.

I would prefer a backward-compatible change here by defaulting to not enabling the observe-only resources feature unless a provider maintainer introduces the corresponding command-line flag & the corresponding machinery for this alpha feature to avoid confusion in those who bump their upjet versions not knowing about this change. A field in config.Provider may control this behavior (defaulting to false so that plain upjet version bumps without introducing an observe-only feature flag will not break anything). What do you think? Or do we want to force the provider maintainers to support observe-only resources in their providers when they bump their upjet versions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just pushed 462dd33 for backward compatibility as you suggested.

Copy link
Collaborator

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

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

Thanks @turkenh, lgtm.

@turkenh turkenh merged commit 4a46ee6 into crossplane:main Apr 13, 2023
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