Skip to content

Discover EKS: allow custom labels for Kube Server #49420

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

Merged
marcoandredinis merged 1 commit into from
Dec 11, 2024
Merged

Discover EKS: allow custom labels for Kube Server #49420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
454 changes: 238 additions & 216 deletions api/gen/proto/go/teleport/integration/v1/awsoidc_service.pb.go
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions api/proto/teleport/integration/v1/awsoidc_service.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,8 @@ message EnrollEKSClustersRequest {
// AgentVersion is version of agent Helm chart to install on the EKS cluster.
// Required.
string agent_version = 5;
// ExtraLabels added to the enrolled clusters.
map<string, string> extra_labels = 6;
}

// EnrollEKSClusterResult contains result for a single cluster enrollment.
Expand Down
1 change: 1 addition & 0 deletions lib/auth/integration/integrationv1/awsoidc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,6 +537,7 @@ func (s *AWSOIDCService) EnrollEKSClusters(ctx context.Context, req *integration
AgentVersion: req.AgentVersion,
TeleportClusterName: clusterName.GetClusterName(),
IntegrationName: req.Integration,
ExtraLabels: req.ExtraLabels,
})
if err != nil {
return nil, trace.Wrap(err)
Expand Down
17 changes: 14 additions & 3 deletions lib/integrations/awsoidc/eks_enroll_clusters.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"encoding/base64"
"fmt"
"log/slog"
"maps"
"net/url"
"slices"
"strings"
Expand Down Expand Up @@ -253,6 +254,9 @@ type EnrollEKSClustersRequest struct {

// AgentVersion specifies version of the Helm chart that will be installed during enrollment.
AgentVersion string

// ExtraLabels added to the enrolled clusters.
ExtraLabels map[string]string
}

// CheckAndSetDefaults checks if the required fields are present.
Expand Down Expand Up @@ -687,13 +691,20 @@ func installKubeAgent(ctx context.Context, cfg installKubeAgentParams) error {
common.ApplyEKSNameSuffix(kubeCluster)
vals["kubeClusterName"] = kubeCluster.GetName()

labels := kubeCluster.GetStaticLabels()
labels[types.InternalResourceIDLabel] = cfg.resourceID
vals["labels"] = labels
vals["labels"] = kubeAgentLabels(kubeCluster, cfg.resourceID, cfg.req.ExtraLabels)

if _, err := installCmd.RunWithContext(ctx, agentChart, vals); err != nil {
return trace.Wrap(err, "could not install Helm chart.")
}

return nil
}

func kubeAgentLabels(kubeCluster types.KubeCluster, resourceID string, extraLabels map[string]string) map[string]string {
labels := make(map[string]string)
maps.Copy(labels, extraLabels)
maps.Copy(labels, kubeCluster.GetStaticLabels())
Comment on lines +705 to +706
Copy link
Copy Markdown
Collaborator

@r0mant r0mant Dec 5, 2024

Choose a reason for hiding this comment

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

I assume "extraLabels" are basically user-provided labels and "static labels" are labels from EKS tags, correct? In that case, should user-defined labels take precedence, i.e. should we swap the order of these two copies?

Copy link
Copy Markdown
Contributor Author

@marcoandredinis marcoandredinis Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I assume "extraLabels" are basically user-provided labels and "static labels" are labels from EKS tags, correct?

Yes 👍

In that case, should user-defined labels take precedence, i.e. should we swap the order of these two copies?

My assumption was that we want static/cloud labels to always be applied after the custom ones, preventing any override of values such as the AWS Account ID/Region.

I looked into how Teleport Server + ServerInfo labels and it seems that its order is:

  • Cloud Labels
  • ServerInfo Labels
  • Static Labels

    teleport/lib/srv/regular/sshserver.go

    Lines 1026 to 1040 in 4846ef4

    func (s *Server) getStaticLabels() map[string]string {
    labels := make(map[string]string, len(s.labels))
    if s.cloudLabels != nil {
    maps.Copy(labels, s.cloudLabels.Get())
    }
    // Let labels sent over ics override labels from instance metadata.
    if s.inventoryHandle != nil {
    maps.Copy(labels, s.inventoryHandle.GetUpstreamLabels(proto.LabelUpdateKind_SSHServerCloudLabels))
    }
    // Let static labels override any other labels.
    maps.Copy(labels, s.labels)
    return labels
    }

Static labels end up being the last to be applied.

Having said that, I'm ok changing the order 👍

labels[types.InternalResourceIDLabel] = resourceID

return labels
}
27 changes: 27 additions & 0 deletions lib/integrations/awsoidc/eks_enroll_clusters_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
eksTypes "github.com/aws/aws-sdk-go-v2/service/eks/types"
"github.com/aws/aws-sdk-go-v2/service/sts"
"github.com/aws/smithy-go/middleware"
"github.com/google/uuid"
"github.com/gravitational/trace"
"github.com/jonboulle/clockwork"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -621,3 +622,29 @@ func (m *mockEnrollEKSClusterClient) PresignGetCallerIdentityURL(ctx context.Con
}

var _ EnrollEKSCLusterClient = &mockEnrollEKSClusterClient{}

func TestKubeAgentLabels(t *testing.T) {
kubeClusterLabels := map[string]string{
"priority": "yes",
"region": "us-east-1",
}
resourceID := uuid.NewString()
Copy link
Copy Markdown
Contributor

@doggydogworld doggydogworld Dec 10, 2024

Choose a reason for hiding this comment

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

Is it necessary to use UUID generation for testing?

Copy link
Copy Markdown
Contributor Author

@marcoandredinis marcoandredinis Dec 11, 2024

Choose a reason for hiding this comment

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

We can use whatever value here 👍
Is it bad to use it? I was trying to use a real value here instead of a static one.

extraLabels := map[string]string{
"priority": "no",
"custom": "yes",
}

got := kubeAgentLabels(
&types.KubernetesClusterV3{Metadata: types.Metadata{Labels: kubeClusterLabels}},
resourceID,
extraLabels,
)

expectedLabels := map[string]string{
"priority": "yes",
"region": "us-east-1",
"custom": "yes",
"teleport.internal/resource-id": resourceID,
}
require.Equal(t, expectedLabels, got)
}
6 changes: 6 additions & 0 deletions lib/web/integrations_awsoidc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,12 +534,18 @@ func (h *Handler) awsOIDCEnrollEKSClusters(w http.ResponseWriter, r *http.Reques
return nil, trace.Wrap(err)
}

extraLabels := make(map[string]string, len(req.ExtraLabels))
Copy link
Copy Markdown
Contributor

@doggydogworld doggydogworld Dec 10, 2024

Choose a reason for hiding this comment

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

Can't we use maps.Clone instead? Since maps.Copy is used I think it might make it more consistent.

Copy link
Copy Markdown
Contributor Author

@marcoandredinis marcoandredinis Dec 11, 2024

Choose a reason for hiding this comment

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

req.ExtraLabels format is not a map.
It's a []ui.Label where ui.Label is a struct with Name, Value.

So, I don't think we can use maps.Clone here

for _, label := range req.ExtraLabels {
extraLabels[label.Name] = label.Value
}

response, err := clt.IntegrationAWSOIDCClient().EnrollEKSClusters(ctx, &integrationv1.EnrollEKSClustersRequest{
Integration: integrationName,
Region: req.Region,
EksClusterNames: req.ClusterNames,
EnableAppDiscovery: req.EnableAppDiscovery,
AgentVersion: agentVersion,
ExtraLabels: extraLabels,
})
if err != nil {
return nil, trace.Wrap(err)
Expand Down
2 changes: 2 additions & 0 deletions lib/web/ui/integration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,8 @@ type AWSOIDCEnrollEKSClustersRequest struct {
ClusterNames []string `json:"clusterNames"`
// EnableAppDiscovery specifies if Teleport Kubernetes App discovery should be enabled inside enrolled clusters.
EnableAppDiscovery bool `json:"enableAppDiscovery"`
// ExtraLabels added to the enrolled clusters.
ExtraLabels []ui.Label `json:"extraLabels"`
}

// EKSClusterEnrollmentResult contains result/error for a single cluster enrollment.
Expand Down