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

kapp deploy adds unnecessary selector to service #381

Closed
nak3 opened this issue Nov 28, 2021 · 17 comments
Closed

kapp deploy adds unnecessary selector to service #381

nak3 opened this issue Nov 28, 2021 · 17 comments
Labels
carvel triage This issue has not yet been reviewed for validity helping with an issue Debugging happening to identify the problem

Comments

@nak3
Copy link

nak3 commented Nov 28, 2021

What steps did you take:

Just create a svc with kapp deploy, for example:

  • svc.yaml
apiVersion: v1
kind: Service
metadata:
  name: knative-local-gateway
spec:
  type: ClusterIP
  selector:
    istio: ingressgateway
  ports:
    - name: http2
      port: 80
      targetPort: 8081
$ kapp deploy --app serving --file svc.yaml 
Target cluster 'https://127.0.0.1:40415' (nodes: kind-control-plane, 1+)

Changes

Namespace  Name                   Kind     Conds.  Age  Op      Op st.  Wait to    Rs  Ri  
default    knative-local-gateway  Service  -       -    create  -       reconcile  -   -  

Op:      1 create, 0 delete, 0 update, 0 noop, 0 exists
Wait to: 1 reconcile, 0 delete, 0 noop

Continue? [yN]: y

10:54:34AM: ---- applying 1 changes [0/1 done] ----
10:54:34AM: create service/knative-local-gateway (v1) namespace: default
10:54:34AM: ---- waiting on 1 changes [0/1 done] ----
10:54:34AM: ok: reconcile service/knative-local-gateway (v1) namespace: default
10:54:34AM: ---- applying complete [1/1 done] ----
10:54:34AM: ---- waiting complete [1/1 done] ----

Succeeded

What happened:

  • The created svc has unnecessary selector kapp.k14s.io/app: "1638062141457992726" as:
$ kubectl get svc knative-local-gateway -o yaml
apiVersion: v1
kind: Service
metadata:
  annotations:
    kapp.k14s.io/identity: v1;default//Service/knative-local-gateway;v1
    kapp.k14s.io/original: '{"apiVersion":"v1","kind":"Service","metadata":{"labels":{"kapp.k14s.io/app":"1638062141457992726","kapp.k14s.io/association":"v1.6bea1a7c93addf362419a0524e5b8621"},"name":"knative-local-gateway","namespace":"default"},"spec":{"ports":[{"name":"http2","port":80,"targetPort":8081}],"selector":{"istio":"ingressgateway","kapp.k14s.io/app":"1638062141457992726"},"type":"ClusterIP"}}'
    kapp.k14s.io/original-diff-md5: 971450d50799efd72d2a697373b60bc9
  creationTimestamp: "2021-11-28T01:54:34Z"
  labels:
    kapp.k14s.io/app: "1638062141457992726"
    kapp.k14s.io/association: v1.6bea1a7c93addf362419a0524e5b8621
  name: knative-local-gateway
  namespace: default
  resourceVersion: "8223"
  uid: b8117f93-8dd6-40b9-87a6-287ae80e5d16
spec:
  clusterIP: 10.96.232.75
  clusterIPs:
  - 10.96.232.75
  internalTrafficPolicy: Cluster
  ipFamilies:
  - IPv4
  ipFamilyPolicy: SingleStack
  ports:
  - name: http2
    port: 80
    protocol: TCP
    targetPort: 8081
  selector:
    istio: ingressgateway
    kapp.k14s.io/app: "1638062141457992726"
  sessionAffinity: None
  type: ClusterIP
status:
  loadBalancer: {}
  • The service stops working due to the unnecessary selector.

What did you expect:

  • The kapp should not add the selector kapp.k14s.io/app: "1638062141457992726".

Environment:

  • kapp version (use kapp --version):

Build on 4b9a9e6

$ kubectl version
Client Version: version.Info{Major:"1", Minor:"22", GitVersion:"v1.22.4", GitCommit:"b695d79d4f967c403a96986f1750a35eb75e75f1", GitTreeState:"clean", BuildDate:"2021-11-17T15:48:33Z", GoVersion:"go1.16.10", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"22", GitVersion:"v1.22.0", GitCommit:"c2b5237ccd9c0f1d600d3072634ca66cefdf272f", GitTreeState:"clean", BuildDate:"2021-08-04T20:01:24Z", GoVersion:"go1.16.6", Compiler:"gc", Platform:"linux/amd64"}
@nak3 nak3 added bug This issue describes a defect or unexpected behavior carvel triage This issue has not yet been reviewed for validity labels Nov 28, 2021
@100mik
Copy link
Contributor

100mik commented Nov 28, 2021

That label is added by kapp to track the association of the resource with an app. And I believe the selection behaviour would get scoped to that particular app as the pods deployed as a part of the app will have the same ownership label as well.

You can however disable the addition of the label by using the kapp.k14s.io/disable-default-label-scoping-rules annotation, something like:

#...
metadata:
  annotations:
    kapp.k14s.io/disable-default-label-scoping-rules: ""
#...

Is this a service that is not supposed to have selectors at all?

@nak3
Copy link
Author

nak3 commented Nov 28, 2021

You can however disable the addition of the label by using the kapp.k14s.io/disable-default-label-scoping-rules annotation, something like:

Thank you. Hmm... We probably can use the workaround but if possible we don't want to add the label as we want to test the same yaml manifests which we will ship.

Is this a service that is not supposed to have selectors at all?

The service supports to have selector as istio: ingressgateway selector must be set. However, it does not support any additional selector as the service points to existing Istio deployments.

@100mik 100mik added helping with an issue Debugging happening to identify the problem and removed bug This issue describes a defect or unexpected behavior labels Nov 28, 2021
@100mik
Copy link
Contributor

100mik commented Dec 1, 2021

Do you think a key in kapp Config which lets you disable label scoping for all resources is something that will help your use case better?

@nak3
Copy link
Author

nak3 commented Dec 1, 2021

Hmm... Honestly I am not sure the answer. cc @dprotaso who introduced kapp in knative serving repo.

Hi @dprotaso 👋 For the context of this issue, kapp inserts an additional selector kapp.k14s.io/app: "1638062141457992726" into k8s service so we need to re-create the k8s service by kubectl after kapp created. Do you think the solution which @100mik suggested could be used?

@dprotaso
Copy link
Contributor

dprotaso commented Dec 1, 2021

I think we can just use the annotation to opt out of this behaviour. We can create an ytt overlay to add it to the resources we need

kapp.k14s.io/disable-default-label-scoping-rules: ""

@100mik
Copy link
Contributor

100mik commented Dec 2, 2021

@nak3 does this method work for you?

(Closing this issue for now. Feel free to reopen it if you feel like we can help you improve this workflow!)

@100mik 100mik closed this as completed Dec 2, 2021
@github-actions github-actions bot removed the carvel triage This issue has not yet been reviewed for validity label Dec 2, 2021
@nak3
Copy link
Author

nak3 commented Dec 2, 2021

@dprotaso Could you please send the PR? I am not familiar with the usage of kapp.

@github-actions github-actions bot added the carvel triage This issue has not yet been reviewed for validity label Dec 2, 2021
@dprotaso
Copy link
Contributor

dprotaso commented Dec 3, 2021

I do wonder, if by default, if the Service doesn't have a matching deployment/pod then the label shouldn't be applied.

@Zebradil
Copy link
Member

I bumped into this issue while deploying kube-prometheus-stack.

I render kube-prometheus-stack helm chart and then pipe the output to kapp deploy.
The source yaml files contain only a Service, but not the corresponding StatefulSet, which is created later by the prometheus-operator. The generated StatefulSet doesn't contain kapp.k14s.io/app label in its template. So, the service points to nowhere.

In general, it seems reasonable to have additional labels added by kapp. But I don't quite understand why selector labels are also altered.

@praveenrewar
Copy link
Member

Hi @Zebradil !

But I don't quite understand why selector labels are also altered.

Thanks for sharing your feedback. Currently we are trying to figure out the best way to disable this. Since the existing kapp apps would already have these selector labels, disabling them would result in immutable field errors for Deployments, so we need to figure out a way to make this backward compatible.
As a first step we have introduced a flag default-label-scoping-rules (in v0.52.0) which can be used to disable the default label scoping rules that are added by kapp.

@Zebradil
Copy link
Member

Thanks @praveenrewar, default-label-scoping-rules flag works for me.

@ringerc
Copy link

ringerc commented Aug 12, 2024

See also #138 and #214

You'll probably want

kapp
      --default-label-scoping-rules=false \
      --apply-default-update-strategy=fallback-on-replace \
      ...args...

to redeploy w/o the added annotations, and force delete if patch fails due to immutable fields.

Anyway, shouldn't kapp be adding this info to resources as an annotation not a label?

ringerc added a commit to ringerc/scrapcode that referenced this issue Aug 12, 2024
work around carvel-dev/kapp#381
where kapp adds unwanted labels that break the kube-prometheus
service selctor
@praveenrewar
Copy link
Member

Anyway, shouldn't kapp be adding this info to resources as an annotation not a label?

@ringerc annotations are not "searchable", i.e. they are not indexed, so labels are used so that kapp can track the resources that are part of an app.

@ringerc
Copy link

ringerc commented Sep 5, 2024

@praveenrewar Makes sense, though it's a bit painful when interacting with operators.

Can kubernetes ApplySets (https://kubernetes.io/blog/2023/05/09/introducing-kubectl-applyset-pruning/) help? They seem to be intended to standardise this sort of resource tracking.

@praveenrewar
Copy link
Member

@ringerc ApplySets also use labels, however I agree that using them would increase the interoperability between different tools (which was one of the goals of introducing ApplySets, other than improving kubectl prune, related KEP).

By the way this issue comes up because kapp also adds some selectors to services, deployments and statefulsets, which is causing the issue, and I agree that this behaviour should have been opt in, but due to backward compatibility we can't just remove it.

@ringerc
Copy link

ringerc commented Sep 12, 2024

An opt-in to remove the addition to selectors would be fantastic and would mostly solve this issue.

@praveenrewar
Copy link
Member

Yep, for the same reason we introduced the flag --default-label-scoping-rules, the problem however is that if we set the default value for it to false, then it would become a breaking change for existing kapp apps out there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
carvel triage This issue has not yet been reviewed for validity helping with an issue Debugging happening to identify the problem
Projects
None yet
Development

No branches or pull requests

6 participants