@detiber

Jason, wouldn't the above append all feature gates that the user didn't specify on the command line and set them to true? say if DynamicKubeletConfig was not set by the user this would override the default from false to true.

please, correct me if i'm wrong.

@neolit123 It will only set the ones that have a default value of true. Without this, updating the default value of the feature has no effect. That said, I'm not a fan of this hack, and will take another look today to see if there is a better way to bubble up the feature default value :)

This is super weird... Why wouldn't we populate the struct from the flag itself?

Is this an artifact of feature-gate mimicking.

I've changed course on this and found a better fix that also does the proper thing for upgrade. I'm still working on fixing the plan and upgrade tests to handle it properly, though

+1 seems like the better solution compared to the extra loop to verify Default: true FGs.

given the DNSType extension i would refactor the above to be a switch / case instead of handling it with if / then. it would then need less line changes once more DNS types are introduced.

Latest updates the conditionals that set printKubeDNS/printCoreDNS and the before/after versions, but since we need to potentially print multiples I think it still made sense to leave the if statements in place for the actual printing of the output.

a map can be used too, but it kind of depends if it's OK to print names in the tabs \t:
this also solves the case where the before and after types differ (?). to my understanding this was possible.

// define names for known DNS types here
var dnsNames = make(map[string]string)
dnsNames[constants.CoreDNS] = "CoreDNS"
dnsNames[constants.KubeDNS] = "KubeDNS"

dnsBefore, isKnownDNSBefore := dnsNames[upgrade.Before.DNSType]
dnsAfter, isKnownDNSAfter := dnsNames[upgrade.After.DNSType]

if isKnownDNSBefore && isKnownDNSAfter {
	fmt.Printf("DNS upgrade\t%s %s\t%s %s\n", dnsBefore, upgrade.Before.DNSVersion, dnsAfter, upgrade.After.DNSVersion)
} else {
	// do we need to print an error?
	fmt.Printf("DNS upgrade encountered unknown type. Before: %s, After: %s\n", upgrade.Before.DNSType, upgrade.After.DNSType)  
}

Oooh, I like that!

Playing around with this a bit, and I think it might make the output a bit confusing compared to the surrounding context. That said, I think the output from the current state of this PR is a bit confusing as well.

I think it would be worth putting some thought into how to properly handle this from a UX perspective, since there are a few different cases that we need to take into account:

• component changes, such as Kube DNS to CoreDNS
• new components added (possibly new default addons that weren't available for the previous version)
• components removed (if we potentially would ever want to deprecate an addon, or if multiple addons are merged into a single addon)

Ideally, in the case that we are replacing a component we should break it out from the component upgrade output and draw attention to it in a way that makes it clear to the user what is happening.

@neolit123 do you have any objections with continuing that discussion on a separate issue that can be addressed as a followup?

@detiber I'm not sure I got the full problem here, but I think that we should consider also to use mutators to "fix" older configuration in order to adapt to the latest change in code.
In this case, when upgrading, if the fetureflag CoreDNS is not set, we should add fetureflag coreDNS=false in order to avoid an unexpected switch to CoreDNS.
WDYT?

@fabriziopandini If this was just a case of changing a default for a choice of options that were going to be supported long term, then I would agree with trying to use some type of a mutator to persist the existing config. However, since Kube DNS is being phased out, I think a user should be required to opt-in in order to keep using it.

Ideally, in the case that we are replacing a component we should break it out from the component upgrade output and draw attention to it in a way that makes it clear to the user what is happening.

@neolit123 do you have any objections with continuing that discussion on a separate issue that can be addressed as a followup?

@detiber sounds good, that would probably be the correct course. 👍

kubernetes/kubeadm#810

Put a // TODO: comment with the notes above this and link to a separate issue please.

@timothysc done

Put a // TODO: comment with the notes above this and link to a separate issue please.

So long as we always used this label even for previous CoreDNS deploys, this is ok.

Yes, this label has been used consistently within kubeadm and this is also consistent with the use in /cluster for use with the dns pod autoscaler used there.