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

CFE-857: Apply user defined tags on created gcp resources #7279

Merged
merged 3 commits into from
Mar 1, 2024

Conversation

bharath-b-rh
Copy link
Contributor

@bharath-b-rh bharath-b-rh commented Jun 25, 2023

PR is for adding user-defined tags to the GCP resources created by installer during cluster creation.

  • Terraform APIs require the TagValue resource Name and it needs to be derived using the parameters OrganizationID, UserTag.Key, UserTag.Value defined by user in install-config.
  • Logic for deriving the TagValues Name is implemented using golang, which happens during install-config validation or while generating the terraform variables.
  • Terraform code has the logic for adding tags to the resources during (for compute resources) and after it's creation(for storage buckets).

@bharath-b-rh bharath-b-rh marked this pull request as draft June 25, 2023 12:55
@bharath-b-rh
Copy link
Contributor Author

/hold openshift/enhancements#1217

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jun 25, 2023
@bharath-b-rh bharath-b-rh force-pushed the cfe-857 branch 2 times, most recently from c6b0254 to e67b432 Compare July 13, 2023 07:33
@bharath-b-rh bharath-b-rh force-pushed the cfe-857 branch 2 times, most recently from 07652d2 to 80da9cd Compare July 21, 2023 07:47
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 22, 2023
@bharath-b-rh bharath-b-rh force-pushed the cfe-857 branch 2 times, most recently from 0170f96 to 5bd3416 Compare July 24, 2023 11:30
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 24, 2023
@patrickdillon
Copy link
Contributor

You will want to separate the go vendoring into a separate commit (like this https://github.com/openshift/installer/blob/master/docs/dev/dependencies.md#go) to make it easier to review

@bharath-b-rh
Copy link
Contributor Author

You will want to separate the go vendoring into a separate commit (like this https://github.com/openshift/installer/blob/master/docs/dev/dependencies.md#go) to make it easier to review

Thank you for reviewing @patrickdillon!

I have split the go mod changes into two for go.mod update and for adding new vendor code.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 2, 2023
@bharath-b-rh bharath-b-rh force-pushed the cfe-857 branch 2 times, most recently from 5658f2b to bfac051 Compare August 2, 2023 10:42
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 2, 2023
@bharath-b-rh bharath-b-rh changed the title CFE-857 : Apply user defined tags on created gcp resources CFE-857: Apply user defined tags on created gcp resources Aug 9, 2023
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Aug 9, 2023
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Aug 9, 2023

@bharath-b-rh: This pull request references CFE-857 which is a valid jira issue.

In response to this:

PR is for adding user-defined tags to the GCP resources created by installer during cluster creation.

  • Terraform APIs require the TagValue resource Name and it needs to be derived using the parameters OrganizationID, UserTag.Key, UserTag.Value defined by user in install-config.
  • Logic for deriving the TagValues Name is implemented using golang, which happens while generating the terraform variables.
  • Terraform code has the logic for adding tags to the resources after it's creation.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@bharath-b-rh
Copy link
Contributor Author

/remove-hold

@bharath-b-rh bharath-b-rh marked this pull request as ready for review August 9, 2023 12:04
@openshift-ci openshift-ci bot removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Aug 9, 2023
@openshift-ci openshift-ci bot requested a review from r4f4 August 9, 2023 12:14
@bharath-b-rh
Copy link
Contributor Author

/retest-required

pkg/asset/installconfig/gcp/usertags.go Outdated Show resolved Hide resolved
pkg/asset/installconfig/gcp/usertags.go Outdated Show resolved Hide resolved
pkg/asset/cluster/tfvars/tfvars.go Outdated Show resolved Hide resolved
@bfournie
Copy link
Contributor

/cc @bfournie

@openshift-ci openshift-ci bot requested a review from bfournie January 30, 2024 20:55
pkg/asset/installconfig/gcp/usertags.go Outdated Show resolved Hide resolved
Comment on lines 187 to 184
if errors.As(err, &gErr) && (gErr.HTTPCode() == http.StatusNotFound ||
gErr.HTTPCode() == http.StatusForbidden) {
logrus.Debugf("does not have permission to access %s tag or does not exist", name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why give an ambiguous message if you can identify the 2 scenarios?

Suggested change
if errors.As(err, &gErr) && (gErr.HTTPCode() == http.StatusNotFound ||
gErr.HTTPCode() == http.StatusForbidden) {
logrus.Debugf("does not have permission to access %s tag or does not exist", name)
if errors.As(err, &gErr) {
switch gErr.HTTPCode() {
case http.StatusNotFound:
logrus.Debugf("tag %s does not exist: %s", name, err.Error())
case http.StatusForbidden:
logrus.Debugf("permission issues accessing tag %s: %s", name, err.Error())
}

Copy link
Contributor Author

@bharath-b-rh bharath-b-rh Feb 1, 2024

Choose a reason for hiding this comment

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

Error returned by the GCP APIs is also similar. I think it's because RBAC can be defined on each tag key and value, and with a given service account, it won't be possible to say whether the tag exists and is inaccessible or it doesn't exist at all, so thought to keep the same. Below error was returned for a tag value which didn't exist.

User [xyz] does not have permission to access tagValues instance [namespaced] (or it may not exist): Permission denied on resource 'openshift/key/value' (or it may not exist).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code updated, new API of cloudresourcemanager package returns StatusForbidden for both non-existent and inaccessible tags.

// getProcessedTags returns the list user-provided validated tags.
func getProcessedTags(mgr TagManager, ic *types.InstallConfig) (map[string]string, error) {
if !processedTags.getState() {
if err := validateUserTags(mgr, ic); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit weird that the definition for validateUserTags lives in the validation.go file and it basically just calls validateAndStoreUserTags which is defined in this file. Also, I don't see why this function has to receive the InstallConfig object if it only needs the project name and the userTags to do its job.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I missed this unnecessary call while incorporating the comments, I will update it.
I thought to pass the InstallConfig object reference and use what's required, instead of passing individual parameters. Please let me know, if needs to be changed.

pkg/asset/installconfig/gcp/usertags.go Outdated Show resolved Hide resolved
pkg/asset/installconfig/gcp/usertags.go Outdated Show resolved Hide resolved
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 1, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 7, 2024
@@ -0,0 +1,83 @@
// Code generated by MockGen. DO NOT EDIT.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code will be merged with mock code in pkg/asset/installconfig/gcp/mock/gcpclient_generated.go after updating google.golang.org/api/cloudresourcemanager from v1 to v3 in the follow up PR.

return dupTags
}

// getCloudResourceServiceForTags returns the client required for querying resource manager resources.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code will be merged with client code in pkg/asset/installconfig/gcp/client.go after updating google.golang.org/api/cloudresourcemanager from v1 to v3 in the follow up PR.

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Feb 8, 2024

@bharath-b-rh: This pull request references CFE-857 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

PR is for adding user-defined tags to the GCP resources created by installer during cluster creation.

  • Terraform APIs require the TagValue resource Name and it needs to be derived using the parameters OrganizationID, UserTag.Key, UserTag.Value defined by user in install-config.
  • Logic for deriving the TagValues Name is implemented using golang, which happens during install-config validation or while generating the terraform variables.
  • Terraform code has the logic for adding tags to the resources during (for compute resources) and after it's creation(for storage buckets).

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@r4f4
Copy link
Contributor

r4f4 commented Feb 8, 2024

LGTM in terms of the code but I haven't tested it myself. I'll leave it to @patrickdillon to give this a last look and tag it.

@patrickdillon
Copy link
Contributor

Generally this looks good to me (but needs a rebase). We are moving away from Terraform and implementing a CAPI-based install pattern (see the enhancement for details). This code is already done so I am fine with merging it, but depending on the progress we make, it is possible we will not be using Terraform for GCP in 4.16. In order to support this feature going forward, it will be necessary for us to implement user tag support in the cluster-api-provider-gcp as it currently does not have support.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 16, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 17, 2024
Copy link
Contributor

openshift-ci bot commented Feb 17, 2024

@bharath-b-rh: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-agent-ha-dualstack 55b949a9ed709be12b8267e3ba3e303dd41c1a69 link false /test e2e-agent-ha-dualstack
ci/prow/okd-e2e-aws-ovn 1f9a0419c86f761c5c8e5f1e9cfe581ac454d0ec link false /test okd-e2e-aws-ovn
ci/prow/okd-scos-e2e-aws-ovn 1f9a0419c86f761c5c8e5f1e9cfe581ac454d0ec link false /test okd-scos-e2e-aws-ovn
ci/prow/terraform-images 1f9a0419c86f761c5c8e5f1e9cfe581ac454d0ec link true /test terraform-images
ci/prow/terraform-verify-vendor 1f9a0419c86f761c5c8e5f1e9cfe581ac454d0ec link true /test terraform-verify-vendor
ci/prow/e2e-aws-ovn-edge-zones d35d9f0bf27ff19aeac503430fc0c15810e9cf22 link false /test e2e-aws-ovn-edge-zones
ci/prow/e2e-aws-custom-security-groups d35d9f0bf27ff19aeac503430fc0c15810e9cf22 link false /test e2e-aws-custom-security-groups
ci/prow/altinfra-e2e-aws-ovn-shared-vpc-edge-zones d35d9f0bf27ff19aeac503430fc0c15810e9cf22 link false /test altinfra-e2e-aws-ovn-shared-vpc-edge-zones
ci/prow/altinfra-e2e-aws-ovn-localzones d35d9f0bf27ff19aeac503430fc0c15810e9cf22 link false /test altinfra-e2e-aws-ovn-localzones
ci/prow/altinfra-e2e-aws-custom-security-groups d35d9f0bf27ff19aeac503430fc0c15810e9cf22 link false /test altinfra-e2e-aws-custom-security-groups
ci/prow/e2e-aws-ovn-shared-vpc-edge-zones d35d9f0bf27ff19aeac503430fc0c15810e9cf22 link false /test e2e-aws-ovn-shared-vpc-edge-zones
ci/prow/altinfra-e2e-aws-ovn-single-node d35d9f0bf27ff19aeac503430fc0c15810e9cf22 link false /test altinfra-e2e-aws-ovn-single-node
ci/prow/e2e-vsphere-zones d35d9f0bf27ff19aeac503430fc0c15810e9cf22 link false /test e2e-vsphere-zones
ci/prow/altinfra-e2e-aws-ovn-wavelengthzones d35d9f0bf27ff19aeac503430fc0c15810e9cf22 link false /test altinfra-e2e-aws-ovn-wavelengthzones
ci/prow/e2e-vsphere-ovn d35d9f0bf27ff19aeac503430fc0c15810e9cf22 link true /test e2e-vsphere-ovn
ci/prow/e2e-aws-ovn-shared-vpc d35d9f0bf27ff19aeac503430fc0c15810e9cf22 link false /test e2e-aws-ovn-shared-vpc
ci/prow/altinfra-e2e-aws-ovn-fips d35d9f0bf27ff19aeac503430fc0c15810e9cf22 link false /test altinfra-e2e-aws-ovn-fips
ci/prow/e2e-aws-ovn-single-node d35d9f0bf27ff19aeac503430fc0c15810e9cf22 link false /test e2e-aws-ovn-single-node
ci/prow/e2e-aws-ovn-imdsv2 d35d9f0bf27ff19aeac503430fc0c15810e9cf22 link false /test e2e-aws-ovn-imdsv2
ci/prow/e2e-aws-ovn-fips d35d9f0bf27ff19aeac503430fc0c15810e9cf22 link false /test e2e-aws-ovn-fips
ci/prow/e2e-vsphere-zones-techpreview d35d9f0bf27ff19aeac503430fc0c15810e9cf22 link false /test e2e-vsphere-zones-techpreview
ci/prow/okd-e2e-aws-ovn-upgrade 1363d39 link false /test okd-e2e-aws-ovn-upgrade
ci/prow/e2e-gcp-ovn-xpn 1363d39 link false /test e2e-gcp-ovn-xpn
ci/prow/e2e-gcp-ovn-shared-vpc 1363d39 link false /test e2e-gcp-ovn-shared-vpc

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@bharath-b-rh
Copy link
Contributor Author

/retest-required

@bharath-b-rh
Copy link
Contributor Author

Generally this looks good to me (but needs a rebase). We are moving away from Terraform and implementing a CAPI-based install pattern (see the enhancement for details). This code is already done so I am fine with merging it, but depending on the progress we make, it is possible we will not be using Terraform for GCP in 4.16. In order to support this feature going forward, it will be necessary for us to implement user tag support in the cluster-api-provider-gcp as it currently does not have support.

Thank you! I have rebased the PR.
Tags support was added in capg as part of this issue, where GCP Cluster and Machine Spec API were updated to support tags. And presently, tags are added to Compute Instance created by the provider, need to check for any other resources managed by it.

@patrickdillon
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 1, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 11fb96e into openshift:master Mar 1, 2024
23 of 26 checks passed
@bharath-b-rh bharath-b-rh deleted the cfe-857 branch March 12, 2024 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants