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.

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

Jump to bottom

fix: add a kusion-control label in svc labels #510

Merged
merged 1 commit into from
Sep 7, 2023
Merged

fix: add a kusion-control label in svc labels #510

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
9 changes: 3 additions & 6 deletions pkg/generator/appconfiguration/generator/workload/network/ports_generator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,17 @@ const (
suffixPublic = "public"
suffixPrivate = "private"

// aliyun SLB annotations, ref: https://help.aliyun.com/zh/ack/ack-managed-and-ack-dedicated/user-guide/add-annotations-to-the-yaml-file-of-a-service-to-configure-clb-instances?spm=a2c4g.11186623.0.0.59e26219ESUbqe
// aliyun SLB annotations, ref: https://help.aliyun.com/zh/ack/ack-managed-and-ack-dedicated/user-guide/add-annotations-to-the-yaml-file-of-a-service-to-configure-clb-instances
aliyunLBSpec = "service.beta.kubernetes.io/alibaba-cloud-loadbalancer-spec"
aliyunSLBS1Small = "slb.s1.small"
kusionControl = "kusionstack.io/control"
)

var (
ErrEmptyAppName = errors.New("app name must not be empty")
ErrEmptyProjectName = errors.New("project name must not be empty")
ErrEmptyStackName = errors.New("stack name must not be empty")
ErrEmptySelectors = errors.New("selectors must not be empty")
ErrEmptyPorts = errors.New("ports must not be empty")
ErrInvalidPort = errors.New("port must be between 1 and 65535")
ErrInvalidTargetPort = errors.New("targetPort must be between 1 and 65535 if exist")
ErrInvalidProtocol = errors.New("protocol must be TCP or UDP")
Expand Down Expand Up @@ -163,16 +163,13 @@ func (g *portsGenerator) generateK8sSvc(public bool, ports []network.Port) *v1.S

// only support Aliyun SLB for now, and set SLB spec by default.
svc.Annotations[aliyunLBSpec] = aliyunSLBS1Small
svc.Labels[kusionControl] = "true"
Copy link
Contributor

Choose a reason for hiding this comment

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

why you need to add this label? K8s Service will Inherit the label that Collaset/Deployment have, does they don't have the label of kusionControl?

Copy link
Member Author

Choose a reason for hiding this comment

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

This label is used to cooperate with the KAFE svc controller to implement gracefully rollout. The KAFE svc controller will only process svcs with this label and it is given in the svc sample before.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know this. But I want to know does this label is not attached to the KAFE workload, Collaset? Does it only need to attach to the service?
This label works only for KAFE system, should not get added under other situation.

Copy link
Member Author

Choose a reason for hiding this comment

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

It only works with the KAFE workload. We will move this label and all Aliyun-related labels to platform.k in the next version

}

return svc
}

func validatePorts(ports []network.Port) error {
if len(ports) == 0 {
return ErrEmptyPorts
}

Copy link
Contributor

Choose a reason for hiding this comment

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

If you don't want to render k8s service, should check the length of ports before new ports generator. If empty, don't new the generator.
Deleting the validation of ports length is not correct, cause the k8s service will still try to get generated but failed.

Copy link
Member Author

Choose a reason for hiding this comment

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

If the ports are nil, the logic below will return immediately. I don't understand why this generator will fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

1, I don't know what is "the logic below" you refer to. Besides, what will get returned? an error, or an empty result?
2, If not k8s svc will get generated, why you new the ports generator? It will generate nothing. Add a judgement in “service generator" is much more concise and correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. The logic below this line. That means the logic in this function. This function will return nil if the ports are empty
  2. Yes, deliberately return before the validation function is more readable

portProtocolRecord := make(map[string]struct{})
for _, port := range ports {
if err := validatePort(&port); err != nil {
Expand Down
3 changes: 2 additions & 1 deletion pkg/generator/appconfiguration/generator/workload/network/ports_generator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,8 @@ func TestPortsGenerator_Generate(t *testing.T) {
Name: "testProject-testStack-testApp-public",
Namespace: "testProject",
Labels: map[string]string{
"test-l-key": "test-l-value",
"test-l-key": "test-l-value",
kusionControl: "true",
},
Annotations: map[string]string{
"test-a-key": "test-a-value",
Expand Down
2 changes: 2 additions & 0 deletions pkg/generator/appconfiguration/generator/workload/service_generator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (

"github.com/stretchr/testify/require"
"gopkg.in/yaml.v3"

"kusionstack.io/kusion/pkg/models"
"kusionstack.io/kusion/pkg/models/appconfiguration/workload"
"kusionstack.io/kusion/pkg/models/appconfiguration/workload/container"
Expand All @@ -31,6 +32,7 @@ metadata:
labels:
app.kubernetes.io/name: foo
app.kubernetes.io/part-of: default
kusionstack.io/control: "true"
name: default-dev-foo-public
namespace: default
spec:
Expand Down
Loading