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

Support for custom cluster dns name #752

Closed

Conversation

unbreakab1e
Copy link
Contributor

@unbreakab1e unbreakab1e commented Jun 12, 2019

Hi! We were investigating the possibility to use Gloo as an API Gateway in a newly provisioned cluster. Suddenly we've stumbled upon an issue with the Basic Routing Example being dysfunctional. It seems that the Helm Chart considers that cluster DNS name is always cluster.local, but in our sandbox cluster (provisioned with Kubespray) we changed it to dev-sandbox.local.
As a result no matter how many routes or virtualservices one will create, envoy would never listen on proxy port 8080 -- simply because it could not connect to control plane and retrieve the configuration.
This PR adds a new value to the chart, k8s.cluster-name, so one might override this default behavior.

BOT NOTES:
resolves #754

@solo-git-bot
Copy link

solo-git-bot bot commented Jun 12, 2019

Waiting for approval from someone in the solo-io org to start testing.

@marcogschmidt
Copy link
Contributor

marcogschmidt commented Jun 12, 2019

Hi @unbreakab1e, thank you for the contribution! There are couple of things that need to be added before this PR can be merged:

  1. You need to add a changelog entry documenting your fix. If you look at the changelog directory at the repository root you will find lots of examples. The appropriate entry type in this case is FIX, which will require you to provide a link to a GitHub issue. I have just created an issue you can link to.
  2. The template value files you updated (values-*-template.yaml) are used to generate the actual value files at build time. The generation happens in install/helm/gloo/generate.go. The template files are parsed to a go struct, updated based on the build context and then serialized back to the final value files. You have to add a new field representing the new k8s.cluster-name attribute to the Config struct in install/helm/gloo/generate/values.go, otherwise it will be ignored.

Could you please address these two issues? Hope the explanation is clear, feel free to ask any question otherwise.

I also found the cluster.local string hardcoded in two of our Gloo plugins (here and here) and am currently looking into how to fix those as well.

Update: since the above two occurrences in the plugins are related to function discovery and not directly to the issue at hand, it is best to address them in a separate PR. I created an issue to track this. If you are interested in looking into this fix we'd be happy to guide you through it.

@marcogschmidt
Copy link
Contributor

/test

@marcogschmidt
Copy link
Contributor

Tests failed because the new variable name contains a hyphen. We use camel case for variables (in accordance with Helm best practices). Could you rename it to clusterName?

@unbreakab1e
Copy link
Contributor Author

Hello @marcogschmidt , thanks for the clarifications, I will address those issues in the upcoming days and update the PR.

@solo-git-bot
Copy link

solo-git-bot bot commented Jun 12, 2019

Issues linked to changelog:
#754

@yuval-k
Copy link
Member

yuval-k commented Jun 12, 2019

/test

@marcogschmidt
Copy link
Contributor

@unbreakab1e build failed with this error:

# github.com/solo-io/gloo/install/helm/gloo/generate
install/helm/gloo/generate/values.go:14:18: undefined: K8s
make: *** [/workspace/gopath/src/github.com/solo-io/gloo/_output/.generated-code] Error 2

The K8s type is undefined, you need to add something like this:

type K8s struct {
  ClusterName string `json:"clusterName"`
}

To verify that everything works you can run make manifest from the project root to generate the value files. Also, I'd run make generated-code -B locally just to make sure that the branch build won't fail on formattatting issues.

unbreakab1e and others added 3 commits June 17, 2019 17:09
* Update Route API
* Make API change non-breaking
* Dep ensure
* Refactor code to adapt to API change
* Merge branch 'master' into update-route-api
* Changelog
* Fix linkerd test
* Fix another test
* Change service destination attribute name
* Temporarily fail translation if proxy  contains service route destination
* Merge branch 'master' into update-route-api
* Fix changelog
* Dep ensure
* Completely define the behavior for the gloo.solo.io/h2_service annotation
* Hybrid upstreams snapshot
* Simplify snapshot
* Hybrid upstream client
* Changelog
* Initial PR feedback
* Codegen
* Simplify function
* Fix race in test
* Fix another test race
* Improve error message
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.

Hardcoded kubernetes cluster name
3 participants