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.

Sign up for GitHub

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

Jump to bottom

MGMT-12839: Add Shielded VMs options to gcp mpool and TF #6698

Merged
merged 4 commits into from
Jan 12, 2023
Merged

MGMT-12839: Add Shielded VMs options to gcp mpool and TF #6698

merged 4 commits into from
Jan 12, 2023

Conversation

eranco74
Copy link
Contributor

@eranco74 eranco74 commented Dec 14, 2022 •
edited
Loading

Add support to Shielded VMs on GCP
Feature link: https://issues.redhat.com/browse/OCPBU-158

barbacbd
barbacbd reviewed Dec 14, 2022
View reviewed changes
data/data/gcp/cluster/master/main.tf Outdated Show resolved Hide resolved
pkg/types/gcp/machinepools.go Show resolved Hide resolved
pkg/types/gcp/machinepools.go Show resolved Hide resolved
@barbacbd
Copy link
Contributor

Looks like you need to check our terraform linting as well as golint.

patrickdillon
patrickdillon reviewed Dec 14, 2022
View reviewed changes
Copy link
Contributor

@patrickdillon patrickdillon left a comment

Choose a reason for hiding this comment

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

In general, this looks good. The commit message should be fleshed out according to our contributing guidelines. Generally I would expect this to be several commits, probably along the lines of: adding type to machine pools, add support in machinesets, and terraform; but this is small enough to be manageable as is.

pkg/types/gcp/machinepools.go Show resolved Hide resolved
data/data/gcp/variables-gcp.tf 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 Dec 16, 2022
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 18, 2022
@eranco74 eranco74 force-pushed the MGMT-12839 branch 2 times, most recently from 01caf9a to 8be023f Compare December 18, 2022 09:23
@eranco74
Copy link
Contributor Author

eranco74 commented Jan 4, 2023

Example usage, update the install-config compute or controlPlane platform field:

compute:
- architecture: amd64
  hyperthreading: Enabled
  name: worker
  platform:
    gcp:
      secureBoot: Enabled
  replicas: 0
controlPlane:
  architecture: amd64
  hyperthreading: Enabled
  name: master
  platform:
    gcp:
       secureBoot: Enabled
  replicas: 1

@eranco74 eranco74 force-pushed the MGMT-12839 branch 2 times, most recently from 3e9cf2c to f9cf4f9 Compare January 10, 2023 21:42
@eranco74
MGMT-12838: go.mod: update github.com/openshift/api + vendor
4c7ab15
@eranco74
pkg: adding SecureBoot option to machinepools
29c8674
@eranco74
pkg: add SecureBoot option to gcp/machines.go
8bdeceb
@eranco74
gcp: add secureBoot option to gcp TF
a0b9c49 
note that the shileded_instance_config options have 3 states:
1. User-specified enabled
2. User-specified disabled
3. Not user-specfiied: default
The shileded_instance_config is defined as dynamic block and will be only in case the user specified it.
This should allow us to get the cloud provider default in case the user
didn't specify the config (and will not require code updates if the cloud provider default changes).
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 11, 2023

@eranco74: 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-openstack 8be023f link false /test e2e-openstack
ci/prow/e2e-ibmcloud-ovn 8be023f link false /test e2e-ibmcloud-ovn
ci/prow/e2e-libvirt 8be023f link false /test e2e-libvirt
ci/prow/e2e-openstack-ovn 8be023f link true /test e2e-openstack-ovn
ci/prow/e2e-vsphere-ovn 8be023f link true /test e2e-vsphere-ovn
ci/prow/e2e-aws-ovn-disruptive a0b9c49 link false /test e2e-aws-ovn-disruptive
ci/prow/e2e-gcp-ovn-shared-vpc a0b9c49 link false /test e2e-gcp-ovn-shared-vpc
ci/prow/okd-e2e-aws-ovn-upgrade a0b9c49 link false /test okd-e2e-aws-ovn-upgrade
ci/prow/e2e-aws-ovn-upgrade a0b9c49 link false /test e2e-aws-ovn-upgrade

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.

@patrickdillon
Copy link
Contributor

/approve
/lgtm
/skip

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 11, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 11, 2023

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 11, 2023
@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD face870 and 2 for PR HEAD a0b9c49 in total

@openshift-merge-robot openshift-merge-robot merged commit 8c83507 into openshift:master Jan 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patrickdillon patrickdillon patrickdillon left review comments

@barbacbd barbacbd barbacbd left review comments

Assignees

@patrickdillon patrickdillon

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@eranco74 @barbacbd @patrickdillon @openshift-ci-robot @openshift-merge-robot