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

Fix bug where coredns 1.8.3 needs more permissions #130

Merged
merged 2 commits into from
Jun 15, 2021
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 118 additions & 18 deletions eks/sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/gruntwork-io/go-commons/collections"
"github.com/gruntwork-io/go-commons/errors"
corev1 "k8s.io/api/core/v1"
rbacv1 "k8s.io/api/rbac/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
k8stypes "k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/kubernetes"
Expand Down Expand Up @@ -60,8 +61,12 @@ const (
componentNamespace = "kube-system"
kubeProxyDaemonSetName = "kube-proxy"
corednsDeploymentName = "coredns"
corednsClusterRoleName = "system:coredns"
corednsConfigMapName = "coredns"
corednsConfigMapConfigKey = "Corefile"

endpointslicesAPIGroup = "discovery.k8s.io"
endpointslicesResource = "endpointslices"
)

// SkipComponentsConfig represents the components that should be skipped in the sync command.
Expand Down Expand Up @@ -250,35 +255,45 @@ func upgradeCoreDNS(
) error {
logger := logging.GetProjectLogger()

targetImage := fmt.Sprintf("602401143452.dkr.ecr.%s.amazonaws.com/eks/coredns:v%s", awsRegion, coreDNSVersion)
currentImage, err := getCurrentDeployedCoreDNSImage(clientset)
logger.Info("Confirming compatibility of coredns configuration with latest version.")
// Need to check config for backwards incompatibility if updating to version >= 1.7.0. The keyword `upstream` was
// removed in 1.7 series of coredns, but is used in earlier versions.
compareVal170, err := semverStringCompare(coreDNSVersion, "1.7.0-eksbuild.1")
if err != nil {
return err
}
if currentImage == targetImage {
logger.Info("Current deployed version matches expected version. Skipping coredns update.")
return nil
// Looking for 1 or 0 here, since we want to know when coreDNSVersion is >= 1.7.0
Copy link
Contributor

@rhoboat rhoboat Jun 15, 2021

Choose a reason for hiding this comment

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

NIT: Since annotated versions are considered <, the comparison is actually against 1.7.0-eksbuild.1, which is < 1.7.0. I don't know if it really matters in this comment, but it strikes me as slightly inaccurate. Do I understand correctly?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it could be argued that it doesn't matter for the sake of this comment, since 1.7.0-eksbuild.1 is still 1.7.0 colloquially.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't really matter, because all the EKS coredns versions are always published with the annotation, so the comment is accurate in the context of EKS.

if compareVal170 >= 0 {
if err := updateCorednsConfigMapFor170Compatibility(clientset); err != nil {
return err
}
} else {
logger.Info("Configuration for coredns is up to date. Skipping configuration reformat.")
}

logger.Info("Confirming compatibility of coredns configuration with latest version.")
corednsConfigMap, err := getCorednsConfigMap(clientset)
if err != nil {
return err
}
// Need to check config for backwards incompatibility if updating to version >= 1.7.0. The keyword upstream was
// removed in 1.7 series of coredns, but is used in earlier versions.
compareVal, err := semverStringCompare(coreDNSVersion, "1.7.0-eksbuild.1")
logger.Info("Confirming compatibility of coredns permissions with latest version.")
// Need to check permissions compatibility if updating to version >= 1.8.3. Starting with 1.8.3, coredns requires
// permissions to list and watch endpoint slices.
compareVal183, err := semverStringCompare(coreDNSVersion, "1.8.3-eksbuild.1")
if err != nil {
return err
}
// Looking for 1 or 0 here, since we want to know when coreDNSVersion is >= 1.7.0
if compareVal >= 0 && strings.Contains(corednsConfigMap.Data[corednsConfigMapConfigKey], "upstream") {
logger.Info("Detected old configuration for coredns. Reformatting configuration to latest.")
if err := removeUpstreamKeywordFromCorednsConfigMap(clientset, corednsConfigMap); err != nil {
if compareVal183 >= 0 {
if err := updateCorednsPermissionsFor183Compatibility(clientset); err != nil {
return err
}
} else {
logger.Info("Configuration for coredns is up to date. Skipping configuration reformat.")
logger.Info("ClusterRole permissions for coredns is up to date. Skipping adjusting ClusterRole permissions.")
}

targetImage := fmt.Sprintf("602401143452.dkr.ecr.%s.amazonaws.com/eks/coredns:v%s", awsRegion, coreDNSVersion)
currentImage, err := getCurrentDeployedCoreDNSImage(clientset)
if err != nil {
return err
}
if currentImage == targetImage {
logger.Info("Current deployed version matches expected version. Skipping coredns update.")
return nil
}

logger.Infof("Upgrading current deployed version of coredns (%s) to match expected version (%s).", currentImage, targetImage)
Expand All @@ -303,6 +318,81 @@ func upgradeCoreDNS(
return nil
}

// updateCorednsConfigMapFor170Compatibility updates the ConfigMap to remove traces of the upstream keyword, which was
// removed starting with 1.7.0.
func updateCorednsConfigMapFor170Compatibility(clientset *kubernetes.Clientset) error {
logger := logging.GetProjectLogger()
corednsConfigMap, err := getCorednsConfigMap(clientset)
if err != nil {
return err
}
if strings.Contains(corednsConfigMap.Data[corednsConfigMapConfigKey], "upstream") {
logger.Info("Detected old configuration for coredns. Reformatting configuration to latest.")
if err := removeUpstreamKeywordFromCorednsConfigMap(clientset, corednsConfigMap); err != nil {
return err
}
}
return nil
}

// updateCorednsPermissionsFor183Compatibility updates the coredns ClusterRole to include permissions that are
// additionally needed starting with 1.8.3.
func updateCorednsPermissionsFor183Compatibility(clientset *kubernetes.Clientset) error {
logger := logging.GetProjectLogger()

corednsClusterRole, err := getCorednsClusterRole(clientset)
if err != nil {
return err
}
// Check if any of the policy rules overlap with list/watch permissions for endpointslices
hasListEndpointSlicesRule, hasWatchEndpointSlicesRule := hasEndpointSlicesPermissions(corednsClusterRole.Rules)
if hasListEndpointSlicesRule && hasWatchEndpointSlicesRule {
// Already have the necessary permissions, so do nothing
return nil
}

logger.Info("coredns ClusterRole does not have enough permissions for 1.8.3. Updating ClusterRole.")

// Construct new rule that contains the necessary permissions
newRule := rbacv1.PolicyRule{
APIGroups: []string{endpointslicesAPIGroup},
Resources: []string{endpointslicesResource},
}
if !hasListEndpointSlicesRule {
newRule.Verbs = append(newRule.Verbs, "list")
}
if !hasWatchEndpointSlicesRule {
newRule.Verbs = append(newRule.Verbs, "watch")
}
corednsClusterRole.Rules = append(corednsClusterRole.Rules, newRule)

// Now save the updated ClusterRole
clusterRoleAPI := clientset.RbacV1().ClusterRoles()
_, err = clusterRoleAPI.Update(context.Background(), corednsClusterRole, metav1.UpdateOptions{})
return errors.WithStackTrace(err)
}

// hasEndpointSlicesPermissions checks if the given rules contain the rule for providing list and watch permissions to
// endpointslices. Returns a 2-tuple where the first element indicates having list permissions, and the latter indicates
// having watch permissions.
func hasEndpointSlicesPermissions(rules []rbacv1.PolicyRule) (bool, bool) {
hasListEndpointSlicesRule := false
hasWatchEndpointSlicesRule := false
for _, rule := range rules {
ruleIsForDiscoveryAPI := collections.ListContainsElement(rule.APIGroups, endpointslicesAPIGroup)
ruleIsForEndpointSlices := collections.ListContainsElement(rule.Resources, endpointslicesResource) || collections.ListContainsElement(rule.Resources, rbacv1.ResourceAll)
ruleIsForList := collections.ListContainsElement(rule.Verbs, "list") || collections.ListContainsElement(rule.Verbs, rbacv1.VerbAll)
ruleIsForWatch := collections.ListContainsElement(rule.Verbs, "watch") || collections.ListContainsElement(rule.Verbs, rbacv1.VerbAll)
if ruleIsForDiscoveryAPI && ruleIsForEndpointSlices && ruleIsForList {
hasListEndpointSlicesRule = true
}
if ruleIsForDiscoveryAPI && ruleIsForEndpointSlices && ruleIsForWatch {
hasWatchEndpointSlicesRule = true
}
}
return hasListEndpointSlicesRule, hasWatchEndpointSlicesRule
}

// getCurrentDeployedCoreDNSImage will return the currently configured coredns image on the deployment.
func getCurrentDeployedCoreDNSImage(clientset *kubernetes.Clientset) (string, error) {
deployment, err := clientset.AppsV1().Deployments(componentNamespace).Get(context.Background(), corednsDeploymentName, metav1.GetOptions{})
Expand Down Expand Up @@ -352,6 +442,16 @@ func getCorednsConfigMap(clientset *kubernetes.Clientset) (*corev1.ConfigMap, er
return configMap, nil
}

// getCorednsClusterRole returns the ClusterRole object for coredns.
func getCorednsClusterRole(clientset *kubernetes.Clientset) (*rbacv1.ClusterRole, error) {
clusterRoleAPI := clientset.RbacV1().ClusterRoles()
corednsClusterRole, err := clusterRoleAPI.Get(context.Background(), corednsClusterRoleName, metav1.GetOptions{})
if err != nil {
return nil, errors.WithStackTrace(err)
}
return corednsClusterRole, nil
}

// getBaseURLForVPCCNIManifest returns the base github URL where the manifest for the VPC CNI is located given the
// requested version.
func getBaseURLForVPCCNIManifest(vpcCNIVersion string) (string, error) {
Expand Down