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.

Sign up for GitHub

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

Jump to bottom

Parametrize the kube-dns --federations command line argument in the manifest #27986

Merged
merged 3 commits into from
Jun 24, 2016
Merged

Parametrize the kube-dns --federations command line argument in the manifest #27986

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c2e70a7
Parametrize the kube-dns --federations command line argument in the m…
madhusudancs Jun 23, 2016
f980aa2
Address review comments.
madhusudancs Jun 23, 2016
3ee03a5
Remove federation_domain_map entirely if it isn't set, seems to be no…
madhusudancs Jun 24, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 18 additions & 0 deletions build/common.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -950,6 +950,24 @@ function kube::release::package_kube_manifests_tarball() {
mkdir -p "${dst_dir}/dns"
tar c -C "${KUBE_ROOT}/cluster/saltbase/salt/kube-dns" ${objects} | tar x -C "${dst_dir}/dns"

# We leave the `{{ pillar['federations_domain_map'] }}` parameter as is, if
# the right federation environment variables isn't set. This is to allow
# users to provide these pillar values using the regular salt's mechanisms
# during cluster bootstrap.
if [[ "${FEDERATION:-}" == "true" ]]; then
FEDERATIONS_DOMAIN_MAP="${FEDERATIONS_DOMAIN_MAP:-}"
if [[ -z "${FEDERATIONS_DOMAIN_MAP}" && -n "${FEDERATION_NAME:-}" && -n "${DNS_ZONE_NAME:-}" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not certain about the order of these shell scripts, but we set FEDERATION_NAME and DNS_ZONE_NAME in

kubernetes/federation/cluster/common.sh

Line 96 in 890ac5f

export FEDERATION_NAME="${FEDERATION_NAME:-federation}"
which I think runs after this code runs.
Maybe move that code to set these env vars here?
Or if the order is not certain, we will have to set the default values for FEDERATION_NAME and DNS_ZONE_NAME at both places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nikhiljindal The operation that defaults DNS_ZONE_NAME (and FEDERATION_NAME) in federation/cluster/common.sh is independent of the operation here. This code here is part of the build and the operation that defaults DNS_ZONE_NAME is part of the deploy. We will have to default the same value twice if we want to default those values.

I don't like defaulting DNS_ZONE_NAME and FEDERATION_NAME here (as opposed to my general inclination towards reasonable defaults) for two reasons:

  1. Doing that will entirely eliminate the users' ability to set those values later during cluster deployment, i.e. during the salt parameter substitution time.
  2. It is not possible to get the e2e tests passing with the current defaults anyway and if a user wants to get their tests to pass they will have to supply non-default values, i.e. the domain name that they own. So there is no reasonable default here.

For the automated tests however, we will provide the FEDERATIONS_DOMAIN_MAP value directly to the Jenkins job. See kubernetes/test-infra#211. So that should take care of the automated tests case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe its because I dont know enough about salt: but why would someone want to override it using salt rather than doing it using env var? The advantage of doing it using env var is that it sets the right values in both kube-dns and the controller manager. (salt will only set it in kube-dns).
The problem I see with current setup is that it can happen that I dont set the values anywhere (neither in env var nor in salt), my cluster bring up will work fine. Just that my DNS resolution for federation queries will not work.
If we decide not to set default values for FEDERATION_NAME and DNS_ZONE_NAME, then cluster bring up script should throw an error if FEDERATION=true and FEDERATION_NAME or DNS_ZONE_NAME are not specified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe its because I dont know enough about salt: but why would someone want to override it using salt rather than doing it using env var?

The code isn't there right now and it could go in a follow up PR, but conceivably this parameter could be replaced by an environment variable on the master node when the master node is configured. That's why I wanted to leave it as is. I am not doing that right now because it caused other problems. But that should be followed up.

If we decide not to set default values for FEDERATION_NAME and DNS_ZONE_NAME, then cluster bring up script should throw an error if FEDERATION=true and FEDERATION_NAME or DNS_ZONE_NAME are not specified.

Sure, the bring up script should throw an error if the values aren't there. We should probably do that. I am not entirely sure if our defaulting is the right thing to do. It makes the tests fail semi-silently.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes as discussed, we should do one: default to a reasonable value or throw an error.
Looks like you dont like defaulting so we can throw an error :)

Dont want to block this PR. Lets get this PR merged to get the tests passing.
You can send a follow up PR.

FEDERATIONS_DOMAIN_MAP="${FEDERATION_NAME}=${DNS_ZONE_NAME}"
fi
if [[ -n "${FEDERATIONS_DOMAIN_MAP}" ]]; then
sed -i 's/{{ pillar\['"'"'federations_domain_map'"'"'\] }}/- --federations="'"${FEDERATIONS_DOMAIN_MAP}"'"/g' "${dst_dir}/dns/skydns-rc.yaml.in"
Copy link
Contributor

Choose a reason for hiding this comment

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

We aren't redirecting stdout anywhere. So this change goes nowhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Whoops, no. sed -i

else
sed -i '/{{ pillar\['"'"'federations_domain_map'"'"'\] }}/d' "${dst_dir}/dns/skydns-rc.yaml.in"
fi
else
sed -i '/{{ pillar\['"'"'federations_domain_map'"'"'\] }}/d' "${dst_dir}/dns/skydns-rc.yaml.in"
fi

# This is for coreos only. ContainerVM, GCI, or Trusty does not use it.
cp -r "${KUBE_ROOT}/cluster/gce/coreos/kube-manifests"/* "${release_stage}/"

Expand Down
1 change: 1 addition & 0 deletions cluster/saltbase/salt/kube-dns/skydns-rc.yaml.base
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ spec:
# command = "/kube-dns"
- --domain=__PILLAR__DNS__DOMAIN__.
- --dns-port=10053
__PILLAR__FEDERATIONS__DOMAIN__MAP__
ports:
- containerPort: 10053
name: dns-local
Expand Down
1 change: 1 addition & 0 deletions cluster/saltbase/salt/kube-dns/skydns-rc.yaml.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ spec:
# command = "/kube-dns"
- --domain={{ pillar['dns_domain'] }}.
- --dns-port=10053
{{ pillar['federations_domain_map'] }}
ports:
- containerPort: 10053
name: dns-local
Expand Down
1 change: 1 addition & 0 deletions cluster/saltbase/salt/kube-dns/transforms2salt.sed
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
s/__PILLAR__DNS__SERVER__/{{ pillar['dns_server'] }}/g
s/__PILLAR__DNS__REPLICAS__/{{ pillar['dns_replicas'] }}/g
s/__PILLAR__DNS__DOMAIN__/{{ pillar['dns_domain'] }}/g
s/__PILLAR__FEDERATIONS__DOMAIN__MAP__/{{ pillar['federations_domain_map'] }}/g
s/__MACHINE_GENERATED_WARNING__/Warning: This is a file generated from the base underscore template file: __SOURCE_FILENAME__/g
1 change: 1 addition & 0 deletions cluster/saltbase/salt/kube-dns/transforms2sed.sed
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
s/__PILLAR__DNS__SERVER__/$DNS_SERVER_IP/g
s/__PILLAR__DNS__REPLICAS__/$DNS_REPLICAS/g
s/__PILLAR__DNS__DOMAIN__/$DNS_DOMAIN/g
/__PILLAR__FEDERATIONS__DOMAIN__MAP__/d
s/__MACHINE_GENERATED_WARNING__/Warning: This is a file generated from the base underscore template file: __SOURCE_FILENAME__/g