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

Adding initial support for GCP #1926

Merged
merged 3 commits into from
Aug 20, 2024

Conversation

beraldoleal
Copy link
Member

@beraldoleal beraldoleal commented Jul 17, 2024

This is basically Cfir' work with me changing paths and small things, to accommodate repository layout changes a few months ago.

We had this on hold because I was working on GKE support as well. However, GKE right now depends on #1920. So my proposal is to merge this, since it will not affect other providers, and when #1920 is merged I will send another PR with the GKE support.

@beraldoleal beraldoleal force-pushed the gcp-without-gke branch 2 times, most recently from c242956 to 12998c5 Compare July 18, 2024 01:41
@beraldoleal beraldoleal force-pushed the gcp-without-gke branch 3 times, most recently from e28605f to d7578e6 Compare July 18, 2024 20:10
@wainersm
Copy link
Member

Hi @beraldoleal !

I've already started my review but I'd like to send a few words before making comments in code:

  • I'm not an expert on GCP, actually I barely know it. So always be suspicious about my comments .
  • I'm largely comparing this implementation with Azure and AWS providers.
  • I assume this implementation is a MVP, so some features (e.g. confidential VMs) are missing on purpose. Even though, I may make some comments so that we start building a gap list (if you haven't such as list already). These comments shouldn't block the merge of this work.
  • I may have some difficult to test it entirely as I don´t have a GCP account (still looking for one). I´ll try my best.

src/peerpod-ctrl/Makefile Outdated Show resolved Hide resolved
@beraldoleal
Copy link
Member Author

beraldoleal commented Jul 19, 2024

Hi @wainersm, thanks for your review! Really appreciated!

Let me know if you are ok with the open threads, and even the closed ones, please feel free to reopen.

About the account, I will send you on slack the credentials to a GCP cluster, if you still need.

@beraldoleal
Copy link
Member Author

@wainersm let me know if you are ok with the changes.

@bpradipt please consider review this when you find some spare cycles.

@beraldoleal beraldoleal force-pushed the gcp-without-gke branch 2 times, most recently from 3960a06 to f2c7c23 Compare August 7, 2024 19:24
Copy link
Member

@wainersm wainersm left a comment

Choose a reason for hiding this comment

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

Hi @beraldoleal !

I see that you followed the instructions in https:/confidential-containers/cloud-api-adaptor/blob/main/src/cloud-api-adaptor/docs/addnewprovider.md which makes me more comfortable to review.

All the comments I made you addressed and/or will on following changes. Unfortunately I don't have GCP account to give it a try but I know that you tested it, so we are fine.

thanks!

@wainersm
Copy link
Member

wainersm commented Aug 7, 2024

@bpradipt @snir911 mind to review too?

@bpradipt
Copy link
Member

bpradipt commented Aug 8, 2024

@bpradipt @snir911 mind to review too?

Yeah, this is in pipeline. Should be doing soon.

.gitignore Show resolved Hide resolved
This is basically Cfir's work with some modifications to support the
repository layout and small fixes.

Signed-off-by: Cfir Cohen <[email protected]>
Signed-off-by: Beraldo Leal <[email protected]>
mkosi uses userdata, necessary here if we are going to avoid packer.

Signed-off-by: Beraldo Leal <[email protected]>
Some basic instructions on how to test and deploy GCP.

Signed-off-by: Cfir Cohen <[email protected]>
Signed-off-by: Beraldo Leal <[email protected]>
Copy link
Member

@bpradipt bpradipt left a comment

Choose a reason for hiding this comment

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

/lgtm
Thanks @beraldoleal

@wainersm wainersm merged commit f9f1208 into confidential-containers:main Aug 20, 2024
20 checks passed
@beraldoleal
Copy link
Member Author

Thank you @wainersm and @bpradipt

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.

3 participants