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

Virtual machine Locality Load Balancing #45413

Closed
2 of 16 tasks
wenfengLee-24 opened this issue Jun 12, 2023 · 15 comments · Fixed by #45476
Closed
2 of 16 tasks

Virtual machine Locality Load Balancing #45413

wenfengLee-24 opened this issue Jun 12, 2023 · 15 comments · Fixed by #45476
Labels
feature/Virtual-machine issues related with VM support

Comments

@wenfengLee-24
Copy link

wenfengLee-24 commented Jun 12, 2023

Is this the right place to submit this?

  • This is not a security vulnerability or a crashing bug
  • This is not a question about how to use Istio

Bug Description

When the virtual machine connects to the istiod, if Locality Load Balancing is used, the istiod will create a label of "istio-locality: region/zone" in the node labels when the proxy establishes a connection with the istiod. However, when the instance automatically registers and goes online, this label will be added to the workloadEntry, but it will trigger the label verification of the workloadEntry, indicating that the value is illegal, causing the creation of the workloadEntry to fail. Because value cannot contain ‘/‘.

Add labels for the agent code :
release-1.17.2-patch/pkg/bootstrap/config.go
var l *core.Locality if meta.Labels[model.LocalityLabel] == "" && options.Platform != nil { // The locality string was not set, try to get locality from platform l = options.Platform.Locality() } else { // replace "." with "/" localityString := model.GetLocalityLabelOrDefault(meta.Labels[model.LocalityLabel], "") if localityString != "" { // override the label with the sanitized value meta.Labels[model.LocalityLabel] = localityString } l = util.ConvertLocality(localityString) }

Verify the labels of the workloadEntry :
release-1.17.2-patch/pkg/config/validation/validation.go :
`func validateWorkloadEntry(we *networking.WorkloadEntry) (Warning, error) {
addr := we.Address
if addr == "" {
return nil, fmt.Errorf("address must be set")
}

// Since we don't know if its meant to be DNS or STATIC type without association with a ServiceEntry,
// check based on content and try validations.
// First check if it is a Unix endpoint - this will be specified for STATIC.
errs := Validation{}
if strings.HasPrefix(addr, UnixAddressPrefix) {
	errs = appendValidation(errs, ValidateUnixAddress(strings.TrimPrefix(addr, UnixAddressPrefix)))
	if len(we.Ports) != 0 {
		errs = appendValidation(errs, fmt.Errorf("unix endpoint %s must not include ports", addr))
	}
} else if !netutil.IsValidIPAddress(addr) { // This could be IP (in STATIC resolution) or DNS host name (for DNS).
	if err := ValidateFQDN(addr); err != nil { // Otherwise could be an FQDN
		errs = appendValidation(errs, fmt.Errorf("endpoint address %q is not a valid FQDN or an IP address", addr))
	}
}

errs = appendValidation(errs,
	labels.Instance(we.Labels).Validate())
for name, port := range we.Ports {
	// TODO: Validate port is part of Service Port - which is tricky to validate with out service entry.
	errs = appendValidation(errs,
		ValidatePortName(name),
		ValidatePort(int(port)))
}
return errs.Unwrap()

}`

Version

istiod 1.17.2

Additional Information

No response

Affected product area

  • Ambient
  • Docs
  • Installation
  • Networking
  • Performance and Scalability
  • Extensions and Telemetry
  • Security
  • Test and Release
  • User Experience
  • Developer Infrastructure
  • Upgrade
  • Multi Cluster
  • Virtual Machine
  • Control Plane Revisions
@istio-policy-bot istio-policy-bot added the feature/Virtual-machine issues related with VM support label Jun 12, 2023
@hzxuzhonghu
Copy link
Member

For WorkloadEntry how "istio-locality" is brought, you set it?

@wenfengLee-24
Copy link
Author

No, it's not what I set up. It's the label added by istiod when the proxy establishes a connection with istiod。
code : release-1.17.2-patch/pkg/bootstrap/config.go. line : 648

@hzxuzhonghu
Copy link
Member

But we where is meta.Labels[model.LocalityLabel] come from in your case?

@wenfengLee-24
Copy link
Author

I am Created a label for istio-locality in ./etc/istio/pod/labels.

@hzxuzhonghu
Copy link
Member

IC, That's what i meant

@wenfengLee-24
Copy link
Author

Sorry, then How to add a locality label when using virtual machine access and Locality Load Balancing .

@hzxuzhonghu
Copy link
Member

There is another way: you can declare locality within workloadEntry.

The issue you reported should be fixed though

@wenfengLee-24
Copy link
Author

We are using automatic registration workloadEntry;
However, generating locality in the workloadEntry requires reading from xdsnode, but the locality data of xdsnode is defined from meta.Labels[model.LocalityLabel]
code : pilot/autoregistration/controller.go line: 603

@hzxuzhonghu
Copy link
Member

got it, then I must fix it

@wenfengLee-24
Copy link
Author

From the code, it can be seen that options.Platform can be read, but how to define options.Platform. Locality

if meta.Labels[model.LocalityLabel] == "" && options.Platform != nil { // The locality string was not set, try to get locality from platform l = options.Platform.Locality() } else { localityString := model.GetLocalityLabelOrDefault(meta.Labels[model.LocalityLabel], "") l = util.ConvertLocality(localityString) }

@hzxuzhonghu
Copy link
Member

@wenfengLee-24 can you test this patch

@wenfengLee-24
Copy link
Author

Okay, I'll notify you after the test is completed

@wenfengLee-24
Copy link
Author

There were no issues with the test, and we can register workloadEntry. @hzxuzhonghu

But will deleting this label meta.Labels[model.LocalityLabel] affect other functions

@hzxuzhonghu
Copy link
Member

No, we donot use labels from WLE

@wenfengLee-24
Copy link
Author

Okay, thank you .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/Virtual-machine issues related with VM support
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants