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

Update the docs for private GKE. #2190

Merged
merged 16 commits into from
Sep 22, 2020
Merged

Conversation

jlewi
Copy link
Contributor

@jlewi jlewi commented Sep 14, 2020

@kubeflow-bot
Copy link

This change is Reviewable

@jlewi jlewi changed the title Update the docs for private GKE. WIP Update the docs for private GKE. Sep 14, 2020
@jlewi
Copy link
Contributor Author

jlewi commented Sep 14, 2020

* Replace the instructions for private GKE with the instructions from the
  repo: https://github.com/kubeflow/gcp-blueprints/blob/master/kubeflow/deploy_private.md

* fix: kubeflow#1705
@jlewi jlewi changed the title WIP Update the docs for private GKE. Update the docs for private GKE. Sep 14, 2020
@jlewi
Copy link
Contributor Author

jlewi commented Sep 14, 2020

/assign @Bobgy
This should be ready for review.

Copy link
Contributor

@Bobgy Bobgy 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!

/assign @joeliedtke
for tech writer review

Comment on lines 81 to 82
* If this times out waiting for the cluster to be ready check if the reason the update failed is
because of [kubeflow/gcp-blueprints#34](https://github.com/kubeflow/gcp-blueprints/issues/35)
Copy link
Contributor

Choose a reason for hiding this comment

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

The issue has been fixed and closed, shall we remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


1. Remove the images from the `images` section
1. Remove it from the `steps` section
* DNS configurations - Check that the `DNSRecordSet` and `DNSManagedZone` CNRM resources are in a ready state
Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like it'll be better if we wait for all CNRM resources before continuing to apply other manifests.
Just a side note

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack.

Copy link
Contributor

@8bitmp3 8bitmp3 left a comment

Choose a reason for hiding this comment

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

Hey @jlewi Thanks again!

I did a quick scan, here are some of my suggestions @Bobgy @joeliedtke :

  • Lines 12-13:
    • "work around" -> "workaround" or "work-around"?
    • For links: "text"
    • Add a short description of the GitHub issue and provide a definition for CRD (CustomResourceDefinition(s) - one word, according to k8s docs):
    • One word "containercluster" -> two words "container cluster"? (I couldn't find containercluster (one word) on Google Cloud docs unless it was a field/var name in some CLI output.)
1. As a workaround for Issue
   [kubeflow/gcp-blueprints#32](https://github.com/kubeflow/gcp-blueprints/issues/32)
   (in CNRM 1.9.1, the [CustomResourceDefinition
   (CRD)](https://kubernetes.io/docs/concepts/extend-kubernetes/api-extension/custom-resources/#customresourcedefinitions)
   for container cluster is missing `ipAllocationPolicy` fields needed to create
   a private GKE cluster), modify the container cluster CRD schema in your
   management cluster to include the missing fields.
  • Add: "where {PKGDIR} is the directory..." (for Kubeflow that you created earlier)?

  • "1. Change to the Kubeflow directory" (<- "kubeflow")

  • Add back ticks: patchesStrategicMerge and resources

  • Add "Note:"

   *Note:* Do not use `kustomize edit` to perform the above actions until Issue [kubernetes-sigs/kustomize#2310](https://github.com/kubernetes-sigs/kustomize/issues/2310) is fixed.
  • Add full stops at the end of sentences.

  • "Sanity check" -> "double check" (less slang).

  • Line 194: Let's add a definition to "GCR", since it could be Google Cloud resources/Run/registry (depending on who's reading this).

  • Line 240: since there is already one troubleshooting section for KF on private GKE:

- * [Troubleshoot](/docs/gke/troubleshooting-gke) any issues you may
  find.
+ * [Troubleshoot](/docs/gke/troubleshooting-gke) any GKE issues you may
  find.

LMKWYT

content/en/docs/gke/private-clusters.md Outdated Show resolved Hide resolved
content/en/docs/gke/private-clusters.md Outdated Show resolved Hide resolved
content/en/docs/gke/private-clusters.md Show resolved Hide resolved
content/en/docs/gke/private-clusters.md Outdated Show resolved Hide resolved
export PROJECT_NUMBER=$(gcloud projects describe ${PROJECT} --format='value(projectNumber)')
```
1. Open `instance/gcp_config`
1. In patchesStrategicMerge add
Copy link
Contributor

@8bitmp3 8bitmp3 Sep 16, 2020

Choose a reason for hiding this comment

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

Suggested change
1. In patchesStrategicMerge add
1. In `patchesStrategicMerge`, add

content/en/docs/gke/private-clusters.md Outdated Show resolved Hide resolved
content/en/docs/gke/private-clusters.md Outdated Show resolved Hide resolved
content/en/docs/gke/private-clusters.md Outdated Show resolved Hide resolved
you will need to add an explict ingress firewall rule to allow that port to be accessed.

These errors usually manifest as failures to create custom resources that depend on webhooks. An example
error is
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
error is
error is:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved.

content/en/docs/gke/private-clusters.md Show resolved Hide resolved
Co-authored-by: 8bitmp3 <19637339+8bitmp3@users.noreply.github.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm label Sep 16, 2020
jlewi and others added 2 commits September 16, 2020 06:24
Co-authored-by: 8bitmp3 <19637339+8bitmp3@users.noreply.github.com>
Co-authored-by: 8bitmp3 <19637339+8bitmp3@users.noreply.github.com>
@jlewi
Copy link
Contributor Author

jlewi commented Sep 16, 2020

@8bitmp3 Thanks for all the great suggestions! I think I addressed all of them (may have missed one or two full stops).

@8bitmp3 I think the only use of containercluster is in a command

kubectl --context=$(MGMTCTXT) wait --for=condition=Ready --timeout=600s  containercluster $(NAME)

In this case there is no space; its one word.

Similarly I think for custom resource defintiion 3 words or 1 are both valid depending on whether you are using the actual resource name as it appears in APIs or more descriptively.

@jlewi
Copy link
Contributor Author

jlewi commented Sep 17, 2020

/test all

@jlewi
Copy link
Contributor Author

jlewi commented Sep 18, 2020

@8bitmp3 and @Bobgy Could one of you PTAL?

Copy link
Contributor

@8bitmp3 8bitmp3 left a comment

Choose a reason for hiding this comment

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

@jlewi Thanks a million!

A few suggestions:

  • Line 10: The steps below 1. Follow the blueprint instructions to setup a management cluster - should be be indented to the right?
  • Line 20: Refactor to:
   * Check Issue [kubeflow/gcp-blueprints#32](https://github.com/kubeflow/gcp-blueprints/issues/32)
     to find out if it has been resolved in later versions of CNRM. If the issue hasn't been resolved, there should be directions for a workaround.

(or something along these lines)

  • Added back ticks around ${PKGDIR} and Makefile.
  • Added full stops (.) and colons (:) in sequential and non-sequential instructions for consistency.
  • Added commas and carried out some minor refactoring.
  • There was a space missing in * Google Container Registry (GCR) images can't be pulled.
  • Changed "Ensure" to "Make sure" (this is more formal, but to me it looks good either way).
  • To avoid repeating "issue" twice, refactored: * This likely indicates an issue with access to private GCR. This could be because of:
  • Added "and" -> * If image pull errors show IP addresses and not the restricted.googleapis.com VIP, then you have an issue with networking.

Obviously, these are just suggestions. LMKWYT!

cc @Bobgy

Cheers

content/en/docs/gke/private-clusters.md Outdated Show resolved Hide resolved
content/en/docs/gke/private-clusters.md Outdated Show resolved Hide resolved
content/en/docs/gke/private-clusters.md Outdated Show resolved Hide resolved
content/en/docs/gke/private-clusters.md Outdated Show resolved Hide resolved
content/en/docs/gke/private-clusters.md Outdated Show resolved Hide resolved

Kubeflow uses IAP to make Kubeflow web apps accessible
from your browser.
1. Follow the blueprint instructions to setup a management cluster
Copy link
Contributor

Choose a reason for hiding this comment

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

Line 10: The steps below 1. Follow the blueprint instructions to setup a management cluster - should be be indented to the right?

content/en/docs/gke/private-clusters.md Outdated Show resolved Hide resolved
content/en/docs/gke/private-clusters.md Outdated Show resolved Hide resolved
content/en/docs/gke/private-clusters.md Outdated Show resolved Hide resolved
content/en/docs/gke/private-clusters.md Outdated Show resolved Hide resolved
@Bobgy
Copy link
Contributor

Bobgy commented Sep 21, 2020

/lgtm
from my side

Co-authored-by: 8bitmp3 <19637339+8bitmp3@users.noreply.github.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm label Sep 21, 2020
jlewi and others added 8 commits September 21, 2020 07:42
Co-authored-by: 8bitmp3 <19637339+8bitmp3@users.noreply.github.com>
Co-authored-by: 8bitmp3 <19637339+8bitmp3@users.noreply.github.com>
Co-authored-by: 8bitmp3 <19637339+8bitmp3@users.noreply.github.com>
Co-authored-by: 8bitmp3 <19637339+8bitmp3@users.noreply.github.com>
Co-authored-by: 8bitmp3 <19637339+8bitmp3@users.noreply.github.com>
Co-authored-by: 8bitmp3 <19637339+8bitmp3@users.noreply.github.com>
Co-authored-by: 8bitmp3 <19637339+8bitmp3@users.noreply.github.com>
@jlewi
Copy link
Contributor Author

jlewi commented Sep 21, 2020

Line 10: The steps below 1. Follow the blueprint instructions to setup a management cluster - should be be indented to the right?

This shouldn't be indented; the instructions below aren't setting up the management cluster. I added a link to the management cluster setup instructions.

@jlewi
Copy link
Contributor Author

jlewi commented Sep 21, 2020

@8bitmp3 Thanks again! I incorporated all your suggestions. PTAL.

@8bitmp3
Copy link
Contributor

8bitmp3 commented Sep 21, 2020

/lgtm

Thank you @jlewi !

Let's have an updated KF on private GKE doc on the site asap, since the current one is out of date. Any incremental improvements can be added later on.

@Bobgy WDYT?

Copy link
Contributor

@8bitmp3 8bitmp3 left a comment

Choose a reason for hiding this comment

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

Hey @jlewi, I ran "private GKE" in the search bar:

image

I think maybe we could improve the UX by amending the title a little bit.

- title = "Securing Your Clusters"
+ title = "Securing Your Clusters with Private GKE"
description = "How to secure Kubeflow clusters using private GKE"

or

- title = "Securing Your Clusters"
+ title = "Securing Your Clusters on Private GKE"
description = "How to secure Kubeflow clusters using private GKE"

It would be similar to:

title = "Kubeflow On-premises on Anthos GKE"

LMKWYT

@@ -1,307 +1,247 @@
+++
title = "Securing Your Clusters"
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @jlewi, I ran "private GKE" in the search bar:

image

I think maybe we could improve the UX by amending the title a little bit.

- title = "Securing Your Clusters"
+ title = "Securing Your Clusters with Private GKE"
description = "How to secure Kubeflow clusters using private GKE"

or

- title = "Securing Your Clusters"
+ title = "Securing Your Clusters on Private GKE"
description = "How to secure Kubeflow clusters using private GKE"

It would be similar to:

title = "Kubeflow On-premises on Anthos GKE"

LMKWYT

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me. I'll merge this PR first because the original one is out of date

@Bobgy
Copy link
Contributor

Bobgy commented Sep 22, 2020

/lgtm
/approve
Thank you both @jlewi @8bitmp3!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bobgy

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

@k8s-ci-robot k8s-ci-robot merged commit c59e3ae into kubeflow:master Sep 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[GCP] Update docs for private clusters and VPC service controls for 1.0
6 participants