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

cloudfunctions: allow to configure trigger_http security level #9583

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

gomezjdaniel
Copy link

@gomezjdaniel gomezjdaniel commented Jul 19, 2021

Current provider doesn't allow to create an only-HTTPS-triggered function.

  • This is not a backwards compatible change
  • Possible values are SECURE_OPTIONAL (http/s) and SECURE_ALWAYS (https)

Relates to #8465

- This is not a backwards compatible change
- Possible values are `SECURE_OPTIONAL` (http/s) and `SECURE_ALWAYS` (https)
@megan07 megan07 requested a review from slevenick July 19, 2021 16:52
Copy link
Collaborator

@slevenick slevenick left a comment

Choose a reason for hiding this comment

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

Unfortunately we won't be able to accept this change.

This would be a breaking change because changing the type of this field from Bool -> String will break existing user's configs if they define this field as a boolean. This is possible to do in a major version of the provider, but unlikely to be something we would do.

I think the better approach would be to add a new optional field that controls this setting

@gomezjdaniel
Copy link
Author

gomezjdaniel commented Jul 19, 2021

@slevenick got it.

I then added a new security_level field as #8465 suggests :)

In case everything is good, I will squash commits afterwards.

Also note cloudfunctions.HttpsTrigger.SecurityLevel possible values are ["SECURITY_LEVEL_UNSPECIFIED", "SECURE_ALWAYS", "SECURE_OPTIONAL"] but I omit the unspecified one.

@slevenick
Copy link
Collaborator

@slevenick got it.

I then added a new security_level field as #8465 suggests :)

In case everything is good, I will squash commits afterwards.

Also note cloudfunctions.HttpsTrigger.SecurityLevel possible values are ["SECURITY_LEVEL_UNSPECIFIED", "SECURE_ALWAYS", "SECURE_OPTIONAL"] but I omit the unspecified one.

I don't think we can include RequiredWith: []string{"trigger_http"}, as that would also make this change backwards incompatible. If the change would cause an existing config to fail it is considered breaking and would not be allowed until a major version of the provider.

Removing that should make this feasible at the cost of some validation

@gomezjdaniel
Copy link
Author

gomezjdaniel commented Jul 21, 2021

My wrong @slevenick, I was trying to make trigger_http a required field in case security_level was set. Otherwise the http trigger would be enabled with either only of the previous fields being set which would make it confusing in my opinion.

Please check my latest commit ;).

@gomezjdaniel gomezjdaniel force-pushed the cloudfunctions/http-trigger-security branch from 53b9c1d to 19d140d Compare July 21, 2021 08:33
@gomezjdaniel gomezjdaniel force-pushed the cloudfunctions/http-trigger-security branch from 19d140d to 252ee33 Compare July 21, 2021 08:34
@gomezjdaniel
Copy link
Author

Ping @slevenick

Copy link
Collaborator

@slevenick slevenick left a comment

Choose a reason for hiding this comment

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

I've upstreamed this to our code generator so it can be replicated across the google and google-beta providers.

We will need docs describing this new field before we can merge this, can you add that? It would go into this file: https://github.com/hashicorp/terraform-provider-google/blob/master/website/docs/r/cloudfunctions_function.html.markdown

@gomezjdaniel
Copy link
Author

Sure, there you go ;). I also fixed a typo in the code.

@gomezjdaniel gomezjdaniel force-pushed the cloudfunctions/http-trigger-security branch from 4708254 to 4de607e Compare August 10, 2021 09:08
@slevenick
Copy link
Collaborator

So it looks like RequiredWith causes the dependency both ways:

Error: RequiredWith
on terraform_plugin_test.tf line 20, in resource "google_cloudfunctions_function" "function":
20:   trigger_http          = true
"trigger_http": all of `security_level,trigger_http` must be specified

which may make this a breaking change. We will likely have to remove that RequiredWith validation

modular-magician added a commit to modular-magician/terraform-provider-google that referenced this pull request Dec 7, 2023
… Source Manager Instance resource (hashicorp#9583)

[upstream:d026cd2d4c705b31429b353b8ef1df25ee18048a]

Signed-off-by: Modular Magician <[email protected]>
modular-magician added a commit that referenced this pull request Dec 7, 2023
… Source Manager Instance resource (#9583) (#16731)

[upstream:d026cd2d4c705b31429b353b8ef1df25ee18048a]

Signed-off-by: Modular Magician <[email protected]>
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.

None yet

2 participants