Add support for configuring public access prevention on Buckets

[nkvoll]

Description of your changes

This PR updates the storage api dependency (and what it cascades to also update) and adds support for configuring the PublicAccessPrevention field on Buckets (see https://cloud.google.com/storage/docs/public-access-prevention)

I removed some potential support for enabling/disabling autopilot on clusters, but I cannot find that this setting is possible to update in newer versions of the libraries, nor any mention of that in the docs (via https://cloud.google.com/kubernetes-engine/docs/concepts/autopilot-overview).

If there is a better way to handle this please let me know.

I was a bit unsure how to best handle the permissible string values whilst avoiding larger refactoring. I figured exposing the raw int enum from the google storage API was undesirable, but I can't see e.g a webhook where the allowed values can be verified early.

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

Running locally, creating and updating Bucket resource with setting publicAccessPrevention to inherited/enforced and seeing it update similarly in the GCP console. Also removing the field reflects the current value from the GCP console.

[nkvoll]

Looking at https://cloud.google.com/config-connector/docs/reference/resource-docs/container/containercluster#autopilot_cluster, it seems to point to enableAutopilot being immutable:

enableAutopilot Optional | boolean Immutable. Enable Autopilot for this cluster.

[nkvoll]

make check-diff passes locally, not sure what's up with the CI panic:

$ make check-diff
23:01:49 [ .. ] verify go modules dependencies have expected content
all modules verified
23:01:50 [ OK ] go modules dependencies verified
23:01:50 [ .. ] go generate linux_arm64
23:01:55 [ OK ] go generate linux_arm64
23:01:55 [ .. ] go mod tidy
23:01:55 [ OK ] go mod tidy
23:01:55 [ .. ] checking that branch is clean
23:01:55 [ OK ] branch is clean

[Feggah]

Thanks for opening this PR, @nkvoll !

I added some small comments below.

[Feggah]

I figured exposing the raw int enum from the google storage API was undesirable, but I can't see e.g a webhook where the allowed values can be verified early.

Based on your code, I believe we should have three options for this field: inherited, enforced or unknown (do we need the unknown value?). Please correct me if I'm wrong.

The unknown value is the default value for the API if you don't specify the PublicAccessPrevention, right?

You can use kubebuilder marker to validate the possible values. Check this example about how to define validation:Enum markers

---

Besides that, I believe that this field is optional, so we should add the // +optional marker and change the field type to be a pointer.

Suggested change

	// PublicAccessPrevention is the setting for the bucket's
	// PublicAccessPrevention policy, which can be used to prevent public access
	// of data in the bucket. See
	// https://cloud.google.com/storage/docs/public-access-prevention for more
	// information.
	PublicAccessPrevention string `json:"publicAccessPrevention,omitempty"`
	// PublicAccessPrevention is the setting for the bucket's
	// PublicAccessPrevention policy, which can be used to prevent public access
	// of data in the bucket. See
	// https://cloud.google.com/storage/docs/public-access-prevention for more
	// information.
	//
	// +optional
	PublicAccessPrevention *string `json:"publicAccessPrevention,omitempty"`

[nkvoll]

Since another PR (#470) has bumped the dependencies version I'm rebasing this PR based the current master branch. Hope this is ok.

Regarding the optionality of the field, I believe having it as the empty string by default is the correct option. Looking at the bucket attributes in the library it is an integer (https://github.com/googleapis/google-cloud-go/blob/main/storage/bucket.go#L345, defaulting to 0, which is the same as "unknown": https://github.com/googleapis/google-cloud-go/blob/main/storage/bucket.go#L485)

The unknown value is the default value for the API if you don't specify the PublicAccessPrevention, right?

It's the default value both sent to and received from the API when the PAP is not specified (which is the case prior to this PR).

The kubebuilder markers is a good idea :) I've added some kubebuilder markers to reflect that it's an enumeration with the possible values for the field. I still specify it as optional as there (possibly) is a sensible default value for this field. Does this make sense to you?

[nkvoll]

Did some testing using this PR:

• Creating a bucket with no publicAccessPrevention in the .spec: Results in the value inherited on my GCP account and the value "inherited" being reflected back to the .spec.publicAccessPrevention field.
• Creating a bucket with publicAccessPrevention: "" in the .spec: Results in the value inherited on my GCP and the value "inherited" being reflected back to the .spec.publicAccessPrevention field.account.

I'm adding a commit to this PR that makes this field optional as you suggested, but for it to make sense I also needed to change the Observe function a little to allow for the setting to only exist in the GCP API and not in the CR spec (see 9bd9a3a#diff-b25ecc0642483145f66b0c3e2fc414c15b740c9e4f9aca54f8bc2aa193488409R145-R154). Please let me know what you think :)

[nkvoll]

This fell off my radar, but AFAIK it's still relevant? @Feggah, if you have time: do you think my changes and the answers to your comments: #467 (comment) and #467 (comment) make sense to you?