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

fix: resolve deprecation warning for binary authorization #1332

Merged

Conversation

wyardley
Copy link
Contributor

enable_binary_authorization is now deprecated in favor of the binary_authorization block. This preserves the module's interface, but updates the underlying behavior

Fixes #1331

@wyardley wyardley requested review from a team, Jberlinsky and bharathkkb as code owners July 22, 2022 06:55
@wyardley wyardley force-pushed the wyardley/issue_1331 branch 4 times, most recently from 56f6daf to e890e1e Compare July 22, 2022 07:50
@wyardley
Copy link
Contributor Author

wyardley commented Jul 22, 2022

Hi @bharathkkb thanks again.
Let me know if the most recent failures have an error you can pass along (the earlier ones got commented on, but this one just shows a failed status check).

I'm not 100% sure if the tests (e.g.,

it "has the expected binaryAuthorization config" do
expect(data['binaryAuthorization']).to eq({
"enabled" => true,
})
) will need to be updated as well, but looks like it still is returned under binaryAuthorization even in fairly recent gcloud versions, so guessing it should be Ok (side note: is it just because of an old version of inspec, or why do most of the tests rely on shellouts to gcloud instead of the native inspec resources?)

@wyardley
Copy link
Contributor Author

Actually, got this after quickly creating a test cluster with binary auth enabled, so updating the test cases as well:

  "binaryAuthorization": {
    "evaluationMode": "PROJECT_SINGLETON_POLICY_ENFORCE"
  },

enable_binary_authorization is now deprecated in favor of the
binary_authorization block. This preserves the module's interface, but
updates the underlying behavior

Fixes terraform-google-modules#1331
@wyardley wyardley force-pushed the wyardley/issue_1331 branch from e890e1e to e533ad5 Compare July 22, 2022 08:26
@comment-bot-dev
Copy link

@wyardley
Thanks for the PR! 🚀
✅ Lint checks have passed.

@wyardley
Copy link
Contributor Author

Also confirmed that a cluster created with the existing version of the module seems to get this:

  "binaryAuthorization": {
    "enabled": true
  },

So we'll need to test whether / how well the upgrade process is (whether it's breaking or not); going to try to do a quick pass to see if I can do some kind of check of that. If not, we could try using the enabled parameter, but just in the binary_authorization block, but that also seems to be deprecated...

@wyardley
Copy link
Contributor Author

wyardley commented Jul 22, 2022

So, I tried creating a cluster with the old version of the module, applying based on my branch. This resulted in the following diff, which was non-destructive and applied cleanly (but slowly).

  # module.gke.google_container_cluster.primary will be updated in-place
  ~ resource "google_container_cluster" "primary" {
      ~ enable_binary_authorization = true -> false
        id                          = "projects/xxx/locations/us-west2-a/clusters/foo"
        name                        = "foo"
        # (26 unchanged attributes hidden)

      ~ addons_config {

          + gke_backup_agent_config {
              + enabled = false
            }

            # (9 unchanged blocks hidden)
        }

      + binary_authorization {
          + evaluation_mode = "PROJECT_SINGLETON_POLICY_ENFORCE"
        }

        # (19 unchanged blocks hidden)
    }

After creating a cluster with the old version of the module:

% gcloud container clusters --zone us-west2-a describe foo  --format=json --project xxx | jq .binaryAuthorization
{
  "enabled": true
}

It's briefly unset during apply, and then after applying the changes:

% gcloud container clusters --zone us-west2-a describe foo  --format=json --project xxx | jq .binaryAuthorization
{
  "evaluationMode": "PROJECT_SINGLETON_POLICY_ENFORCE"
}

Would be great if someone could confirm also that this is the correct setting, though from what I can tell, I think this is the right behavior.

I assume this is Ok to roll out as a "non breaking" change? But may still cause some user confusion, since the new setting is kind of non-obviously named. Also, is it expected that the old resource attribute gets toggled to false vs null in the terraform plan? (the actual result at the API level is fine)

@wyardley
Copy link
Contributor Author

added a docs fix to provide at least slightly more information in the provider docs
GoogleCloudPlatform/magic-modules#6320

Copy link
Member

@bharathkkb bharathkkb left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @wyardley and thanks for testing out the upgrade path!

@bharathkkb bharathkkb merged commit f8a5cca into terraform-google-modules:master Jul 26, 2022
CPL-markus pushed a commit to WALTER-GROUP/terraform-google-kubernetes-engine that referenced this pull request Jul 15, 2024
…google-modules#1332)

enable_binary_authorization is now deprecated in favor of the
binary_authorization block. This preserves the module's interface, but
updates the underlying behavior

Fixes terraform-google-modules#1331
CPL-markus pushed a commit to WALTER-GROUP/terraform-google-kubernetes-engine that referenced this pull request Jul 15, 2024
@wyardley wyardley deleted the wyardley/issue_1331 branch September 13, 2024 19:21
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.

Deprecation warning on enable_binary_authorization
3 participants