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
Add GKE Security Features to Deployment Manager config #879
Conversation
# kubectl at that point. So we put all resources in a single deployment. | ||
- name: kubeflow | ||
type: cluster.jinja | ||
properties: | ||
zone: us-east1-d | ||
# Set this to v1beta1 to use beta features such as private clusters, | ||
apiVersion: v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v1beta1 or v1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to set a default value of v1
. If the end-user wants to leverage beta features, they can set this to v1beta1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the GKE API version? Should we call it gkeApiVersion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Updated
docs/gke/configs/cluster.jinja
Outdated
nodeConfig: | ||
machineType: n1-standard-1 | ||
serviceAccount: {{ properties['serviceAccountName'] }}@{{ env['project'] }}.iam.gserviceaccount.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to use the service account as the VM service account? I think a better practice is to move in the direction of creating secrets containing service account keys. This way we can provide finer grained permissions then what we get from node service accounts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This service account will not be used by pods. This service account is intended to replace the default compute engine service account which has Project Editor Role. We will be using secrets and service account keys to give access to individual pods.
docs/gke/configs/cluster.jinja
Outdated
@@ -72,59 +83,85 @@ resources: | |||
monitoringService: monitoring.googleapis.com/kubernetes | |||
{% else %} | |||
initialClusterVersion: 1.9.6-gke.1 | |||
{% endif %} | |||
{% endif %} | |||
podSecurityPolicyConfig: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make all the security parameters fields in a dictionary "securityFeatures" just to make the properties a little bit more human readable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Looks like the test failed because you need the IAM API; you just need to add
and then make sure the patch step depends on that step. |
Same error
/test all |
Use gke-default oauth scopes Update to gkeApiVersion Add securityConfig Enable iam api in dm
docs/gke/configs/cluster.jinja
Outdated
- https://www.googleapis.com/auth/logging.write | ||
- https://www.googleapis.com/auth/monitoring | ||
- https://www.googleapis.com/auth/service.management.readonly | ||
- https://www.googleapis.com/auth/servicecontrol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logging and monitoring make sense for the logging and monitoring agents but what about the others?
I assume devstorage is for GCR?
Why do we need service.management, servicecontrol, and trace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I substituted gke-default
[1] with the actual scopes. I will reduce the scopes.
[1] https://cloud.google.com/sdk/gcloud/reference/container/clusters/create
docs/gke/configs/cluster.jinja
Outdated
@@ -139,7 +144,12 @@ resources: | |||
machineType: n1-standard-8 | |||
serviceAccount: {{ properties['vmServiceAccountName'] }}@{{ env['project'] }}.iam.gserviceaccount.com | |||
oauthScopes: | |||
- gke-default | |||
- https://www.googleapis.com/auth/devstorage.read_only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we define a variable and reuse it to avoid code duplication and ensure they stay in sync?
@jlewi This is ready for review |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jlewi 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 |
* Add GKE Security Features to bootstrapper Use gke-default oauth scopes Update to gkeApiVersion Add securityConfig Enable iam api in dm * Add oauth scopes manually. gke-default does not work * Fix oauthscopes * Add PodSecurityPolicy only to v1beta1
* feat: Add event when the reconcile is failed Signed-off-by: Ce Gao <gaoce@caicloud.io> * fix: Use format Signed-off-by: Ce Gao <gaoce@caicloud.io>
…ad (kubeflow#879) * image gcr.io/kubeflow-images-public/notebook-controller:vmaster-g0cb184ad * Image built from kubeflow/kubeflow@0cb184ad
Add option for
/cc @jlewi @kunmingg
This change is