CORS-4259, CORS-4260, CORS-4265: Move the gcp permission check to a common file in install config. #10018
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@barbacbd: This pull request references CORS-4259 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.21.0" version, but no target version was set. In response to this: 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. |
|
/hold This will require changes to CAPG version 1.10.x |
openshift-ci
bot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
barbacbd
changed the title
|
@barbacbd: This pull request references CORS-4259 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.21.0" version, but no target version was set. This pull request references CORS-4260 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.21.0" version, but no target version was set. In response to this: 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. |
|
/jira refresh |
|
@barbacbd: This pull request references CORS-4259 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.21.0" version, but no target version was set. This pull request references CORS-4260 which is a valid jira issue. In response to this:
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. |
|
/jira refresh |
|
@barbacbd: This pull request references CORS-4259 which is a valid jira issue. This pull request references CORS-4260 which is a valid jira issue. In response to this:
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. |
|
/retest-required |
|
/retest-required |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/retest |
openshift-merge-robot
added
the
needs-rebase
pkg/types/gcp/validation/platform.go
Outdated
| if p.FirewallRulesManagement == gcp.UnmanagedFirewallRules && p.Network == "" { | ||
| allErrs = append(allErrs, field.Required(fldPath.Child("network"), "a network must be specified when firewall rules are unmanaged")) | ||
| } |
There was a problem hiding this comment.
I think we also need a static validation check to make sure the user input is valid (i.e. equals Managed or Unmanaged) correct me if that's already handled but I think we need to add it explicitly
There was a problem hiding this comment.
I'm fine handling that in a follow up PR
|
/retest-required |
pkg/types/gcp/platform.go: Add FirewallManagementPolicy. The policy will indicate whether the cluster or user will manage the firewall rules. Add validation to ensure that a network is provided when the install config is set to Unmanaged to FirewallManagement. pkg/types/gcp/metadata.go: Add the management policy to the metadata so that the bootstrap destroy process knows whether to delete the bootstrap firewall rules or not.
|
|
||
| // validateFirewallPermissions validates that the user has firewall permissions OR when the user does not have | ||
| // permissions to create firewall rules then a network is specified. | ||
| func validateFirewallPermissions(client API, ic *types.InstallConfig, fieldPath *field.Path) field.ErrorList { |
There was a problem hiding this comment.
I think we still need this validation (or something close to it) to ensure that users that don't have firewall rules have set Unmanaged. We can do that in a follow up PR
|
/lgtm Let's add the permission check validation in a follow up PR. |
tthvo
left a comment
tthvo
left a comment
There was a problem hiding this comment.
/lgtm
+1 for validations (but in another PR). I repeated the same install step and the installation worked as expected 🥳
|
/verified by @patrickdillon |
|
/skip |
openshift-ci-robot
added
the
verified
|
@patrickdillon: This PR has been marked as verified by In response to this:
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. |
|
/override ci/prow-e2e-aws-ovn ci/prow/e2e-aws-ovn-edge-zones-manifest-validation Not relevant tests, with unrelated failures |
|
@patrickdillon: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. In response to this:
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-sigs/prow repository. |
|
/override ci/prow/e2e-aws-ovn |
|
@tthvo: Overrode contexts on behalf of tthvo: ci/prow/e2e-aws-ovn In response to this:
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-sigs/prow repository. |
|
/override ci/prow/e2e-aws-ovn-edge-zones-manifest-validation |
|
@tthvo: Overrode contexts on behalf of tthvo: ci/prow/e2e-aws-ovn-edge-zones-manifest-validation In response to this:
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-sigs/prow repository. |
|
@barbacbd: The following tests failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
|
/override ci/prow/e2e-aws-ovn ci/prow/e2e-aws-ovn-edge-zones-manifest-validation |
|
@patrickdillon: Overrode contexts on behalf of patrickdillon: ci/prow/e2e-aws-ovn, ci/prow/e2e-aws-ovn-edge-zones-manifest-validation In response to this:
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-sigs/prow repository. |
|
/override ci/prow/e2e-azure-ovn test passed |
|
@patrickdillon: Overrode contexts on behalf of patrickdillon: ci/prow/e2e-azure-ovn In response to this:
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-sigs/prow repository. |
|
/override ci/prow/e2e-aws-ovn job is irrelevant and permafailing due to rhel repo causing baremetal build failure |
|
@patrickdillon: Overrode contexts on behalf of patrickdillon: ci/prow/e2e-aws-ovn In response to this:
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-sigs/prow repository. |
|
/override ci/prow/e2e-aws-ovn ci/prow/e2e-aws-ovn-edge-zones-manifest-validation job is irrelevant and permafailing due to rhel repo causing baremetal build failure |
|
@patrickdillon: Overrode contexts on behalf of patrickdillon: ci/prow/e2e-aws-ovn, ci/prow/e2e-aws-ovn-edge-zones-manifest-validation In response to this:
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-sigs/prow repository. |
5 participants
No description provided.