Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Switch to shared gRPC server implementation#166

Merged
ulucinar merged 3 commits into
crossplane-contrib:mainfrom
ulucinar:fix-tj-261
Apr 19, 2022
Merged

Switch to shared gRPC server implementation#166
ulucinar merged 3 commits into
crossplane-contrib:mainfrom
ulucinar:fix-tj-261

Conversation

@ulucinar
Copy link
Copy Markdown
Collaborator

Description of your changes

Fixes crossplane/terrajet#261

Switches to the shared gRPC server implementation in provider-jet-azure.

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Tested using the corresponding Terrajet PR: crossplane/terrajet#267

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
@ulucinar ulucinar force-pushed the fix-tj-261 branch 3 times, most recently from f9b9d48 to 74d633c Compare April 6, 2022 01:16
Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
Copy link
Copy Markdown
Member

@muvaf muvaf left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @ulucinar !

Comment thread cmd/provider/main.go
"sigs.k8s.io/controller-runtime/pkg/log/zap"

tjcontroller "github.com/crossplane/terrajet/pkg/controller"
"github.com/crossplane/terrajet/pkg/terraform"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the crossplane-runtime imports should be moved to this group.

Comment thread internal/clients/azure.go
fmt.Sprintf(fmtEnvVar, envClientID, azureCreds[keyAzureClientID]),
fmt.Sprintf(fmtEnvVar, envClientSecret, azureCreds[keyAzureClientSecret]),
}
ps.Configuration[keyTenantID] = azureCreds[keyAzureTenantID]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you confirm that these are not printed in the error logs?

Copy link
Copy Markdown
Collaborator Author

@ulucinar ulucinar Apr 19, 2022

Choose a reason for hiding this comment

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

Hi @muvaf,
I gave this a try by injecting an invalid key in Terraform provider configuration block. Here's what appears in managed resource status:

  - message: 'observe failed: cannot run refresh: refresh failed: Extraneous JSON
      object property: No argument or block type is named "non-existent".: File name:
      main.tf.json'
    reason: ReconcileError
    status: "False"
    type: Synced

And no relevant provider logs unless debug logging is enabled. If debug logs are enabled, then I think we are dumping Terraform output, which may, in theory, leak sensitive data. We can discuss this general issue in Terrajet.

ARG TERRAFORM_VERSION
ARG TERRAFORM_PROVIDER_SOURCE
ARG TERRAFORM_PROVIDER_VERSION
ARG TINI_VERSION
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Was this a remnant of something else?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, looks like it's no longer used.

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
@ulucinar ulucinar merged commit 13f6186 into crossplane-contrib:main Apr 19, 2022
@ulucinar ulucinar deleted the fix-tj-261 branch April 19, 2022 22:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow Terrajet-based providers to run in shared gRPC mode

2 participants