OCPBUGS-48469: GCP: Update /etc/hosts file when ClusterHostedDNS is enabled #4800
Conversation
sadasu
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
jira/severity-critical
|
@sadasu: This pull request references Jira Issue OCPBUGS-48469, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
sadasu
force-pushed
the
gcp-update-etc-hosts
branch
9 times, most recently
from
1bc96ca to
3280ff8
Compare
| apiServerIntURL={{ .Infra.Status.APIServerInternalURL }} | ||
| # Add the/etc/hosts configuration file | ||
| mkdir -p /etc/hosts/conf.d | ||
| cat <<EOF | tee /etc/hosts/conf.d/etc-hosts.conf |
There was a problem hiding this comment.
Could maybe name the file api.conf or something.
There was a problem hiding this comment.
I was hoping that naming it as etc-hosts.conf would be it obvious that this file contains some configuration for /etc/hosts. Happy to call it api.conf is it contains information about resolving the API/-Int urls.
| {{ else }} | ||
| exit 0 | ||
| {{ end }} | ||
| if [ -z "${apiIntLBIPs}" ]; then |
There was a problem hiding this comment.
Should this be:
| if [ -z "${apiIntLBIPs}" ]; then | |
| if [ -z "{{$apiIntLBIPs}}" ]; then |
?
We appear to be missing a line like:
apiIntLBIPs={{$apiIntLBIPs}}
to get the template variable that we defined on line 19 into a bash variable.
| mkdir -p /etc/hosts/conf.d | ||
| cat <<EOF | tee /etc/hosts/conf.d/etc-hosts.conf | ||
| # Added by OpenShift | ||
| ${apiLBIPs[0]} ${apiServerURL} |
There was a problem hiding this comment.
We are treating apiLBIPs as a bash array here. That's fine if it is, but bash arrays always seem like kind of a pain to set up to me. It might be simpler to do this in the template:
| ${apiLBIPs[0]} ${apiServerURL} | |
| {{$apiLBIPs[0]}} ${apiServerURL} |
| @@ -0,0 +1,11 @@ | |||
| mode: 0755 | |||
| path: "/usr/local/bin/update-etc-hosts" | |||
| contents: | |||
There was a problem hiding this comment.
Potentially a simpler way, avoiding the script and systemd service:
path: "/etc/hosts"
append:
- inline: |
{{ if and (eq .Infra.Status.PlatformStatus.Type "GCP") (.Infra.Status.PlatformStatus.GCP) (.Infra.Status.PlatformStatus.GCP.CloudLoadBalancerConfig) (eq .Infra.Status.PlatformStatus.GCP.CloudLoadBalancerConfig.DNSType "ClusterHosted") }}
{{ $apiIntLBIPs := cloudPlatformAPIIntLoadBalancerIPs . }}
{{ if len $apiIntLBIPs > 0 }}
{{ $apiLBIPs := cloudPlatformAPILoadBalancerIPs . }}
{{ if len $apiLBIPs > 0 }}{{ $apiLBIPs[0] }}{{ else }}{{ $apiIntLBIPs[0] }}{{ end }} {{ .Infra.Status.APIServerURL }}
{{ $apiIntLBIPs[0] }} {{ .Infra.Status.APIServerInternalURL }}
{{ end }}
{{ end }}
There was a problem hiding this comment.
The systemd service is providing us a way to time the running of this script before kubelet.
To check if feature to run in-cluster DNS on GCP and AWS is enabled by checking if the value of `PlatformStatus.GCP.CloudLoadBalancerConfig.DNSType` is set to `ClusterHosted`.
sadasu
force-pushed
the
gcp-update-etc-hosts
branch
2 times, most recently
from
15c0227 to
3d451a6
Compare
sadasu
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
sadasu
force-pushed
the
gcp-update-etc-hosts
branch
4 times, most recently
from
7ff1573 to
91d44ca
Compare
Append /etc/hosts files with entries to resolve cluster api and api-int URLS. /etc/hosts will provide resolution for these URLs until kubelet joins the cluster and runs its CoreDNS pod which will then take over resolution of those 2 URLs
Added tests to accomodate GCP in-cluster DNS config
sadasu
force-pushed
the
gcp-update-etc-hosts
branch
from
91d44ca to
1b13a9c
Compare
| @@ -777,6 +778,33 @@ func cloudPlatformIngressLoadBalancerIPs(cfg RenderConfig) (interface{}, error) | |||
| } | |||
| } | |||
|
|
|||
| // cloudPlatformLBIPAvailable returns true when DNSType is set to `ClusterHosted` | |||
There was a problem hiding this comment.
Curious, based on the comment I'd expect some check for clusterhosted in the function. I guess it's implicit since the service enablement is dependent on this field?
(I know we do the same elsewhere in the template rendering, so I'm fine with it as is)
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sadasu, yuqi-zhang 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 |
openshift-ci
bot
added
the
approved
|
@sadasu: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@sadasu Hi, I just tried to use the latest commit to test the installation of GCP custom DNS. The |
Append /etc/hosts files with entries to resolve cluster api and api-int URLS. /etc/hosts will provide resolution for these URLs until kubelet joins the cluster and runs its CoreDNS pod which will then take over resolution of those 2 URLs
- What I did
- How to verify it
- Description for the changelog