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

Use annotations instead of labels on created k8s resources #4466

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
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
4 changes: 2 additions & 2 deletions integration/helm_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@ func TestHelmDeploy(t *testing.T) {
env := []string{fmt.Sprintf("TEST_NS=%s", ns.Name)}
skaffold.Deploy("--images", "gcr.io/k8s-skaffold/skaffold-helm").InDir("testdata/helm").InNs(ns.Name).WithEnv(env).RunOrFail(t)

// check if labels are set correctly for deployment
// check if annotations are set correctly for deployment
dep := client.GetDeployment("skaffold-helm-" + ns.Name)
testutil.CheckDeepEqual(t, dep.Name, dep.ObjectMeta.Labels["release"])
testutil.CheckDeepEqual(t, "helm", dep.ObjectMeta.Labels["skaffold.dev/deployer"])
testutil.CheckDeepEqual(t, "helm", dep.ObjectMeta.Annotations["skaffold.dev/deployer"])

skaffold.Delete().InDir("testdata/helm").InNs(ns.Name).WithEnv(env).RunOrFail(t)
}
22 changes: 12 additions & 10 deletions integration/render_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ spec:
expectedOut: `apiVersion: v1
kind: Pod
metadata:
labels:
annotations:
Copy link
Contributor

@tejal29 tejal29 Jul 14, 2020

Choose a reason for hiding this comment

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

shd we still have these annotations? We are not using any of the annotations for driving any metrics.
With the new MetadataEvent we are capturing all these metrics via IDEs

Copy link
Contributor

@tejal29 tejal29 Jul 15, 2020

Choose a reason for hiding this comment

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

I remember these were initially added to get some metrics on how folks are using skaffold. I might be wrong.
I don't think any of our users are using these labels to query objects. Removing these won't be a backward compatible change.
We also have --label field for users to optionally add user labels.
WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you meant removing them won't be a backwards incompatible change? we're already removing them as labels which would theoretically break people who are using those labels to filter resources, but I think we're fine to do that.

curious point though as to whether we should even have them. putting them as annotations makes them silent metadata, so I'm inclined to just leave them even though they may be useless, but I also don't care much and can remove them if you think we should

Copy link
Contributor

@tejal29 tejal29 Jul 16, 2020

Choose a reason for hiding this comment

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

I think you meant removing them won't be a backwards incompatible change?

Correct.

curious point though as to whether we should even have them. putting them as annotations makes them silent metadata, so I'm inclined to just leave them even though they may be useless, but I also don't care much and can remove them if you think we should

Removing them could simplify the code a lot. We don't need the Annotations method in the interface. Merging annotations etc is not needed.

skaffold.dev/deployer: kubectl
name: my-pod-123
namespace: default
Expand Down Expand Up @@ -105,7 +105,7 @@ spec:
expectedOut: `apiVersion: v1
kind: Pod
metadata:
labels:
annotations:
skaffold.dev/deployer: kubectl
name: my-pod-123
namespace: default
Expand Down Expand Up @@ -150,7 +150,7 @@ spec:
expectedOut: `apiVersion: v1
kind: Pod
metadata:
labels:
annotations:
skaffold.dev/deployer: kubectl
name: my-pod-123
namespace: default
Expand All @@ -162,7 +162,7 @@ spec:
apiVersion: v1
kind: Pod
metadata:
labels:
annotations:
skaffold.dev/deployer: kubectl
name: my-pod-456
namespace: default
Expand Down Expand Up @@ -528,14 +528,15 @@ spec:
expectedOut: `apiVersion: v1
kind: Pod
metadata:
labels:
app.kubernetes.io/managed-by: SOMEDYNAMICVALUE
annotations:
skaffold.dev/builder: local
skaffold.dev/cleanup: "true"
skaffold.dev/deployer: kubectl
skaffold.dev/docker-api-version: SOMEDYNAMICVALUE
skaffold.dev/run-id: SOMEDYNAMICVALUE
skaffold.dev/tag-policy: git-commit
labels:
app.kubernetes.io/managed-by: SOMEDYNAMICVALUE
skaffold.dev/run-id: SOMEDYNAMICVALUE
name: my-pod-123
spec:
containers:
Expand Down Expand Up @@ -636,14 +637,15 @@ resources:
expectedOut: `apiVersion: v1
kind: Pod
metadata:
labels:
app.kubernetes.io/managed-by: SOMEDYNAMICVALUE
annotations:
skaffold.dev/builder: local
skaffold.dev/cleanup: "true"
skaffold.dev/deployer: kustomize
skaffold.dev/docker-api-version: SOMEDYNAMICVALUE
skaffold.dev/run-id: SOMEDYNAMICVALUE
skaffold.dev/tag-policy: git-commit
labels:
app.kubernetes.io/managed-by: SOMEDYNAMICVALUE
skaffold.dev/run-id: SOMEDYNAMICVALUE
this-is-from: kustomization.yaml
name: my-pod-123
spec:
Expand Down
2 changes: 2 additions & 0 deletions pkg/skaffold/build/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ type Artifact struct {
type Builder interface {
Labels() map[string]string

Annotations() map[string]string

Build(ctx context.Context, out io.Writer, tags tag.ImageTags, artifacts []*latest.Artifact) ([]Artifact, error)

// Prune removes images built with Skaffold
Expand Down
4 changes: 4 additions & 0 deletions pkg/skaffold/build/cluster/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ func NewBuilder(runCtx *runcontext.RunContext) (*Builder, error) {

// Labels are labels specific to cluster builder.
func (b *Builder) Labels() map[string]string {
return map[string]string{}
}

func (b *Builder) Annotations() map[string]string {
return map[string]string{
constants.Labels.Builder: "cluster",
}
Expand Down
3 changes: 2 additions & 1 deletion pkg/skaffold/build/cluster/types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,8 @@ func TestNewBuilder(t *testing.T) {
}

func TestLabels(t *testing.T) {
testutil.CheckDeepEqual(t, map[string]string{"skaffold.dev/builder": "cluster"}, (&Builder{}).Labels())
testutil.CheckDeepEqual(t, map[string]string{}, (&Builder{}).Labels())
testutil.CheckDeepEqual(t, map[string]string{"skaffold.dev/builder": "cluster"}, (&Builder{}).Annotations())
}

func TestPruneIsNoop(t *testing.T) {
Expand Down
4 changes: 4 additions & 0 deletions pkg/skaffold/build/gcb/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,10 @@ func NewBuilder(runCtx *runcontext.RunContext) *Builder {

// Labels are labels specific to Google Cloud Build.
func (b *Builder) Labels() map[string]string {
return map[string]string{}
}

func (b *Builder) Annotations() map[string]string {
return map[string]string{
constants.Labels.Builder: "google-cloud-build",
}
Expand Down
4 changes: 4 additions & 0 deletions pkg/skaffold/build/local/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,10 @@ func (b *Builder) PushImages() bool {

// Labels are labels specific to local builder.
func (b *Builder) Labels() map[string]string {
return map[string]string{}
nkubala marked this conversation as resolved.
Show resolved Hide resolved
}

func (b *Builder) Annotations() map[string]string {
labels := map[string]string{
constants.Labels.Builder: "local",
}
Expand Down
4 changes: 4 additions & 0 deletions pkg/skaffold/build/tag/custom.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ type CustomTag struct {
}

func (c *CustomTag) Labels() map[string]string {
return map[string]string{}
}

func (c *CustomTag) Annotations() map[string]string {
return map[string]string{
constants.Labels.TagPolicy: "custom",
}
Expand Down
4 changes: 4 additions & 0 deletions pkg/skaffold/build/tag/date_time.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ func NewDateTimeTagger(format, timezone string) Tagger {
}

func (tagger *dateTimeTagger) Labels() map[string]string {
return map[string]string{}
}

func (tagger *dateTimeTagger) Annotations() map[string]string {
return map[string]string{
constants.Labels.TagPolicy: "dateTimeTagger",
}
Expand Down
4 changes: 4 additions & 0 deletions pkg/skaffold/build/tag/env_template.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ func NewEnvTemplateTagger(t string) (Tagger, error) {
}

func (t *envTemplateTagger) Labels() map[string]string {
return map[string]string{}
}

func (t *envTemplateTagger) Annotations() map[string]string {
return map[string]string{
constants.Labels.TagPolicy: "envTemplateTagger",
}
Expand Down
5 changes: 5 additions & 0 deletions pkg/skaffold/build/tag/git_commit.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,11 @@ func NewGitCommit(prefix, variant string) (*GitCommit, error) {

// Labels are labels specific to the git tagger.
func (c *GitCommit) Labels() map[string]string {
return map[string]string{}
}

// Labels are labels specific to the git tagger.
func (c *GitCommit) Annotations() map[string]string {
return map[string]string{
constants.Labels.TagPolicy: "git-commit",
}
Expand Down
5 changes: 5 additions & 0 deletions pkg/skaffold/build/tag/sha256.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ type ChecksumTagger struct{}

// Labels are labels specific to the sha256 tagger.
func (c *ChecksumTagger) Labels() map[string]string {
return map[string]string{}
}

// Labels are labels specific to the sha256 tagger.
func (c *ChecksumTagger) Annotations() map[string]string {
return map[string]string{
constants.Labels.TagPolicy: "sha256",
}
Expand Down
2 changes: 2 additions & 0 deletions pkg/skaffold/build/tag/tag.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ type Tagger interface {
// Labels produces labels to indicate the used tagger in deployed pods.
Labels() map[string]string

Annotations() map[string]string

// GenerateFullyQualifiedImageName resolves the fully qualified image name for an artifact.
// The workingDir is the root directory of the artifact with respect to the Skaffold root,
// and imageName is the base name of the image.
Expand Down
8 changes: 6 additions & 2 deletions pkg/skaffold/deploy/helm.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,10 @@ func NewHelmDeployer(runCtx *runcontext.RunContext) *HelmDeployer {

// Labels returns the Kubernetes labels used by this deployer
func (h *HelmDeployer) Labels() map[string]string {
return map[string]string{}
}

func (h *HelmDeployer) Annotations() map[string]string {
return map[string]string{
constants.Labels.Deployer: "helm",
}
Expand Down Expand Up @@ -143,8 +147,8 @@ func (h *HelmDeployer) Deploy(ctx context.Context, out io.Writer, builds []build

event.DeployComplete()

labels := merge(h.addSkaffoldLabels, h, labellers...)
labelDeployResults(labels, dRes)
labels, annotations := merge(h.addSkaffoldLabels, h, labellers...)
labelAndAnnotateDeployResults(labels, annotations, dRes)

// Collect namespaces in a string
namespaces := make([]string, 0, len(nsMap))
Expand Down
6 changes: 5 additions & 1 deletion pkg/skaffold/deploy/kubectl.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,10 @@ func NewKubectlDeployer(runCtx *runcontext.RunContext) *KubectlDeployer {
}

func (k *KubectlDeployer) Labels() map[string]string {
return map[string]string{}
}

func (k *KubectlDeployer) Annotations() map[string]string {
return map[string]string{
constants.Labels.Deployer: "kubectl",
}
Expand Down Expand Up @@ -298,7 +302,7 @@ func (k *KubectlDeployer) renderManifests(ctx context.Context, out io.Writer, bu
}
}

manifests, err = manifests.SetLabels(merge(k.addSkaffoldLabels, k, labellers...))
manifests, err = manifests.SetLabelsAndAnnotations(merge(k.addSkaffoldLabels, k, labellers...))
if err != nil {
return nil, fmt.Errorf("setting labels in manifests: %w", err)
}
Expand Down
47 changes: 28 additions & 19 deletions pkg/skaffold/deploy/kubectl/labels.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ import (
)

// SetLabels add labels to a list of Kubernetes manifests.
func (l *ManifestList) SetLabels(labels map[string]string) (ManifestList, error) {
replacer := newLabelsSetter(labels)
func (l *ManifestList) SetLabelsAndAnnotations(labels, annotations map[string]string) (ManifestList, error) {
replacer := newLabelsAndAnnotationsSetter(labels, annotations)

updated, err := l.Visit(replacer)
if err != nil {
Expand All @@ -36,45 +36,54 @@ func (l *ManifestList) SetLabels(labels map[string]string) (ManifestList, error)
return updated, nil
}

type labelsSetter struct {
labels map[string]string
type labelsAndAnnotationsSetter struct {
labels map[string]string
annotations map[string]string
}

func newLabelsSetter(labels map[string]string) *labelsSetter {
return &labelsSetter{
labels: labels,
func newLabelsAndAnnotationsSetter(labels, annotations map[string]string) *labelsAndAnnotationsSetter {
return &labelsAndAnnotationsSetter{
labels: labels,
annotations: annotations,
}
}

func (r *labelsSetter) Visit(o map[string]interface{}, k string, v interface{}) bool {
func (r *labelsAndAnnotationsSetter) Visit(o map[string]interface{}, k string, v interface{}) bool {
if k != "metadata" {
return true
}

if len(r.labels) == 0 {
return false
}

metadata, ok := v.(map[string]interface{})
if !ok {
return true
}

l, present := metadata["labels"]
if visitAndReplace("labels", r.labels, &metadata) {
return true
}
return visitAndReplace("annotations", r.annotations, &metadata)
}

func visitAndReplace(fieldName string, values map[string]string, metadata *map[string]interface{}) bool {
if len(values) == 0 {
return false
}

field, present := (*metadata)[fieldName]
if !present {
metadata["labels"] = r.labels
(*metadata)[fieldName] = values
return false
}

labels, ok := l.(map[string]interface{})
existingValues, ok := field.(map[string]interface{})
if !ok {
return true
}

for k, v := range r.labels {
// Don't replace existing label.
if _, present := labels[k]; !present {
labels[k] = v
for k, v := range values {
// Don't overwrite existing values
if _, present := existingValues[k]; !present {
existingValues[k] = v
}
}

Expand Down
Loading