ARO-9570: Add a controller to the ARO operator to lay down etc hosts machine config #3771
Conversation
lranjbar
requested review from
jewzaam,
bennerv,
hawkowl,
rogbas,
petrkotas,
jharrington22,
cblecker,
cadenmarchese,
UlrichSchlueter,
SudoBrendan,
yjst2012,
jaitaiwan,
anshulvermapatel,
hlipsig,
tiguelu,
SrinivasAtmakuri,
mociarain,
AldoFusterTurpin,
kimorris27 and
tsatam
as code owners
There was a problem hiding this comment.
LGTM broadly speaking, but I asked a few questions about parts I'm uncertain about. It would also be nice to have E2E tests, though they could probably go in a separate PR if we're in a rush.
Also, I don't think we can run E2E unless you reopen your PR using a branch directly off of Azure/ARO-RP. I'll copy my review comments over to the new PR once you're opened it to keep things organized in one place.
| b, err := json.Marshal(ignConfig) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // canonicalise the machineconfig payload the same way as MCO | ||
| var i interface{} | ||
| err = json.Unmarshal(b, &i) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| rawExt := runtime.RawExtension{} | ||
| rawExt.Raw, err = json.Marshal(i) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &mcfgv1.MachineConfig{ | ||
| TypeMeta: metav1.TypeMeta{ | ||
| APIVersion: mcfgv1.SchemeGroupVersion.String(), | ||
| Kind: "MachineConfig", | ||
| }, | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: fmt.Sprintf("99-%s-aro-etc-hosts-gateway-domains", role), | ||
| Labels: map[string]string{ | ||
| "machineconfiguration.openshift.io/role": role, | ||
| }, | ||
| }, | ||
| Spec: mcfgv1.MachineConfigSpec{ | ||
| Config: rawExt, | ||
| }, | ||
| }, nil |
There was a problem hiding this comment.
Not strictly necessary for this PR if we're in a rush to get this merged, but this portion seems like it could be factored out into a shared utility function that accepts the ignition config, the role, and the MachineConfig name and returns the MachineConfig.
| ) | ||
|
|
||
| const ( | ||
| EtcHostsMachineConfigControllerName = "DnsmasqMachineConfig" |
There was a problem hiding this comment.
I'm not super up-to-speed with the full incident response plan, but I guess this name choice indicates that this controller is intended to completely replace the existing dnsmasq MachineConfig controller?
If so, does it need the additional controllers for watching the ARO cluster CR and the MCPs (like the existing controllers in pkg/operator/controllers/dnsmasq)?
There was a problem hiding this comment.
No, its actually just that I missed this rename. This code its based off the dnsmasq controller. It's not super clear to me why that controller also watches those things. When you place the machine config MCO will reconcile the MCPs. There should be no reason to watch the CR at all.
|
/azp run ci |
|
/azp run e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
No pipelines are associated with this pull request. |
@lranjbar yea - kipp is right - can you move this to a branch under ARO-RP so we can run our pipelines? This is a result of security hardening... you can run |
|
Opened #3781 to merge from branch inside the repo |
Which issue this PR addresses:
Fixes ARO-9570
What this PR does / why we need it:
This PR adds a MachineConfigs for the ARO operator to lay down:
99-master-aro-etc-hosts-gateway-domains
99-worker-aro-etc-hosts-gateway-domains
During the UDR installation incident we have gone back and forth on how resolve the root issue. The root cause is that the gateway domains are not populated for OpenShift images to be pulled and installed. These domains are IPs so we can pass them into
/etc/hoststo resolve them before other DNS resolution options are available.Test plan for issue:
I've some minimal smoke testing on the operator with this PR.
Is there any documentation that needs to be updated for this PR?
How do you know this will function as expected in production?