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

Updates to iap component to support private clusters #1396

Merged
merged 1 commit into from Aug 22, 2018
Merged

Updates to iap component to support private clusters #1396

merged 1 commit into from Aug 22, 2018

Conversation

ankushagarwal
Copy link
Contributor

@ankushagarwal ankushagarwal commented Aug 21, 2018

Docs changes are here : kubeflow/website#173

  • Creates a new component: Ingress Setup
  • Updates all various ingress related bash scripts to use Ingress Setup docker image
  • Don't create certificate CRD when using private cluster
  • Add privateGKECluster and ingressSetupImage flags to iap prototype

/cc @jlewi @danisla


This change is Reviewable

@@ -0,0 +1,3 @@
FROM google/cloud-sdk:alpine

RUN apk add --update jq
Copy link
Contributor

@danisla danisla Aug 21, 2018

Choose a reason for hiding this comment

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

We should also move the curl to install kubectl (and bump it to 1.11) from the shell script to the Dockerfile.

[ -z ${CLIENT_ID} ] && echo Error CLIENT_ID must be set && exit 1
[ -z ${CLIENT_SECRET} ] && echo Error CLIENT_SECRET must be set && exit 1
[ -z ${NAMESPACE} ] && echo Error NAMESPACE must be set && exit 1
[ -z ${SERVICE} ] && echo Error SERVICE must be set && exit 1

apk add --update jq
curl https://storage.googleapis.com/kubernetes-release/release/v1.9.4/bin/linux/amd64/kubectl > /usr/local/bin/kubectl && chmod +x /usr/local/bin/kubectl
Copy link
Contributor

Choose a reason for hiding this comment

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

Add this curl to the Dockerfile and delete from this script.

@@ -9,7 +9,6 @@
[ -z ${NAMESPACE} ] && echo Error NAMESPACE must be set && exit 1
[ -z ${SERVICE} ] && echo Error SERVICE must be set && exit 1

apk add --update jq
curl https://storage.googleapis.com/kubernetes-release/release/v1.9.4/bin/linux/amd64/kubectl > /usr/local/bin/kubectl && chmod +x /usr/local/bin/kubectl
Copy link
Contributor

Choose a reason for hiding this comment

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

Add this curl to the Dockerfile and delete from this script. Also bump it to v1.11.0.

Copy link
Contributor

@danisla danisla left a comment

Choose a reason for hiding this comment

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

Just a couple suggestions, otherwise looks good to me.

@ankushagarwal
Copy link
Contributor Author

Done

Copy link
Contributor

@danisla danisla left a comment

Choose a reason for hiding this comment

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

/LGTM

@@ -0,0 +1,10 @@
# IAP Enabler

IAP Enabler is a docker image which is used to enable iap. We build this and push it to gcr so that it can be used in private GKE clusters
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we use BackendConfig now to enable IAP. I think this is only used to set the timeout on the loadbalancer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll rebase this once #1327 gets merged

@jlewi
Copy link
Contributor

jlewi commented Aug 22, 2018

nit: please update PR description to describe changes that are needed.

@k8s-ci-robot k8s-ci-robot removed the lgtm label Aug 22, 2018
@ankushagarwal
Copy link
Contributor Author

This is updated and ready for review

@danisla
Copy link
Contributor

danisla commented Aug 22, 2018

@ankushagarwal it looks like we now omit installing the Certificate resource for private clusters, should we also omit installing the cert-manager component for private clusters since it won't be used?

@ankushagarwal
Copy link
Contributor Author

I've added it to docs here : https://master.kubeflow.org/docs/started/getting-started-gke/ Remove components which are not useful in private clusters

@danisla
Copy link
Contributor

danisla commented Aug 22, 2018

Great!

/LGTM

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

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 2e305f2 into kubeflow:master Aug 22, 2018
@ankushagarwal ankushagarwal deleted the private-cluster-iap-changes branch August 23, 2018 03:04
saffaalvi pushed a commit to StatCan/kubeflow that referenced this pull request Feb 11, 2021
yanniszark pushed a commit to arrikto/kubeflow that referenced this pull request Feb 15, 2021
* Remove v1alpha3 files

* Modify SDK

* Change dict() to object
surajkota pushed a commit to surajkota/kubeflow that referenced this pull request Jun 13, 2022
* Migrate AWS manifest to v3 pattern

* Clean up tests files

* Add istio namespace to istio ingress

* Update KFP pipeline test case for aws stack
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.

None yet

5 participants