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

Add support to include c2p authority #34

Merged
merged 9 commits into from
Nov 22, 2022

Conversation

arvindbr8
Copy link
Member

@arvindbr8 arvindbr8 commented Nov 8, 2022

As part of xds federation changes, we need to add support to the bootstrap generator to add c2p authority to the config. This change addresses that.

what's happenin?

  1. add C2P TD authority option using a include-directpath-authority-experimental flag
  2. add additional logic to the bootstrap generator to query http://metadata.google.internal/computeMetadata/v1/instance/network-interfaces/0/ipv6s and set the TRAFFICDIRECTOR_DIRECTPATH_C2P_IPV6_CAPABLE to true or false, in node metadata

Part of #32

@arvindbr8
Copy link
Member Author

@easwars please take a look

main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@easwars easwars assigned arvindbr8 and unassigned easwars Nov 8, 2022
@arvindbr8 arvindbr8 assigned easwars and unassigned arvindbr8 Nov 8, 2022
main.go Outdated Show resolved Hide resolved
main.go Outdated
@@ -51,6 +52,7 @@ var (
gceVM = flag.String("gce-vm-experimental", "", "GCE VM name to use, instead of reading it from the metadata server. This flag is EXPERIMENTAL and may be changed or removed in a later release.")
configMesh = flag.String("config-mesh-experimental", "", "Dictates which Mesh resource to use. This flag is EXPERIMENTAL and may be changed or removed in a later release.")
includeFederationSupport = flag.Bool("include-federation-support-experimental", false, "whether or not to generate configs required for xDS Federation. This flag is EXPERIMENTAL and may be changed or removed in a later release.")
includeC2PAuthority = flag.Bool("include-c2p-authority-experimental", false, "whether or not to include c2p TD authority for xDS Federation. Ignored if not used with include-federation-support-experimental flag. This flag is EXPERIMENTAL and may be changed or removed in a later release.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

c2p might not be very descriptive for the users. Maybe include-directpath-authority-experimental. Wdyt @ejona86 ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

meh. Users should never need to set this. It'll just become the default and then go away.

main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@easwars easwars assigned arvindbr8 and unassigned easwars Nov 8, 2022
@arvindbr8
Copy link
Member Author

I'm also updating this to include changes to support adding node metadata indicating IPv6 capabilities for DirectPath b/258291928

@arvindbr8 arvindbr8 assigned easwars and unassigned arvindbr8 Nov 9, 2022
@arvindbr8 arvindbr8 added the enhancement New feature or request label Nov 9, 2022
Copy link
Collaborator

@easwars easwars left a comment

Choose a reason for hiding this comment

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

I think changes are mostly looking good. Some very minor comments this time around.

main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main_test.go Outdated Show resolved Hide resolved
main_test.go Outdated Show resolved Hide resolved
@easwars easwars assigned arvindbr8 and unassigned easwars and ejona86 Nov 11, 2022
main.go Outdated Show resolved Hide resolved
main_test.go Outdated Show resolved Hide resolved
main_test.go Outdated Show resolved Hide resolved
main_test.go Outdated Show resolved Hide resolved
Copy link
Member Author

@arvindbr8 arvindbr8 left a comment

Choose a reason for hiding this comment

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

rev'ing the PR addressing comments. @easwars pls take a look when you can

main.go Outdated Show resolved Hide resolved
main_test.go Outdated Show resolved Hide resolved
main_test.go Outdated Show resolved Hide resolved
main_test.go Outdated Show resolved Hide resolved
@arvindbr8 arvindbr8 assigned easwars and unassigned arvindbr8 Nov 15, 2022
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main_test.go Outdated Show resolved Hide resolved
@easwars easwars assigned arvindbr8 and unassigned easwars Nov 15, 2022
@arvindbr8 arvindbr8 assigned easwars and unassigned arvindbr8 Nov 15, 2022
main.go Outdated
@@ -371,12 +382,15 @@ func getFromMetadata(urlStr string) ([]byte, error) {
if err != nil {
return nil, fmt.Errorf("failed reading from metadata server: %w", err)
}
if code := resp.StatusCode; code < 200 || code > 299 {
return nil, fmt.Errorf("failed reading from metadata server with Non-OK status code: %d", code)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit on the actual error message. Include the url since this function is shared and a concise error string.

return nil, fmt.Errorf("metadata server returned status code %d for url %q", code, url)

Copy link
Member Author

Choose a reason for hiding this comment

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

yup that makes sense.. I'll update that

@easwars easwars assigned ejona86 and arvindbr8 and unassigned easwars Nov 15, 2022
@easwars
Copy link
Collaborator

easwars commented Nov 15, 2022

@ejona86 : I think the changes look good. I'm assigning you for a second set of eyes. Thank you.

@arvindbr8 arvindbr8 removed their assignment Nov 17, 2022
@arvindbr8 arvindbr8 merged commit 8431bfc into GoogleCloudPlatform:master Nov 22, 2022
@arvindbr8 arvindbr8 deleted the c2pauthority branch November 22, 2022 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants