-
Notifications
You must be signed in to change notification settings - Fork 184
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 setup.sh to use Terraform #249
Conversation
- name: kubectl | ||
image: gcr.io/google_containers/kubectl:v1.0.7 | ||
command: ["/kubectl"] | ||
args: ["proxy", "-p", "8001"] |
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.
Need to ask @frankreno about potential security concern on exposing a port in the container. Is there other ways to initialize the kubernetes
provider?
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.
That was only intended to be for the demo I have you guys. We will need to support proper authentication and see if we can do this - https://www.terraform.io/docs/providers/kubernetes/index.html#authentication
@@ -14,18 +14,14 @@ metadata: | |||
spec: | |||
template: | |||
spec: | |||
restartPolicy: Never | |||
restartPolicy: OnFailure |
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.
Will this cause it to never eventually die?
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.
yeah, my thinking was we shouldn't proceed if the setup fails, since it will be missing collectors/sources/namespace/secret. By restarting on failure, it gives them the chance to modify the config map to fix the issue -OR- cancel the helm install and skip setupEnabled
if they want to manage it manually.
volumeMounts: | ||
- name: setup | ||
mountPath: /etc/terraform | ||
env: |
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 may not need to be solved in this PR, but this new process (and existing) do not work when there is a proxy involved. However, Terraform supports using a proxy by setting HTTP_PROXY/HTTPS_PROXY environment variables. Would be good to expose that.
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.
LGTM, left a few comments
Thanks for reviewing, I plan on adding one small change today to account for customers that set endpoint URL including After that, we will need to wait for a Helm chart release that has the updated Docker image with the TF providers before merging. |
Description
Adds Terraform-based setup for Sumo collector and HTTP sources, as well as Kubernetes namespace and secret.
Requires #248 to be merged, and a Helm chart release (minor version, non-alpha) so that the docker image contains the required dependencies.
Do not merge until we have a Helm chart release with the updated DockerfileDockerfile changes are in Helm chart release 0.10.0
Testing performed