-
Notifications
You must be signed in to change notification settings - Fork 60
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
feat: CSE-based bootstrapping with bootstrapping client mode #527
base: main
Are you sure you want to change the base?
Changes from 14 commits
cd66fd2
06d9613
d8ff982
7169f33
c6a76ba
5743723
dc5f626
62e5c3b
8440647
425a55a
45cd0c0
049640c
ff4706a
a5c8403
a57bb00
aadbe7b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
#!/bin/bash | ||
|
||
# Define the file path | ||
FILE="pkg/provisionclients/client/operations/node_bootstrapping_get_responses.go" | ||
|
||
# Check if the file exists | ||
if [ ! -f "$FILE" ]; then | ||
echo "File $FILE does not exist." | ||
exit 1 | ||
fi | ||
|
||
# Use sed to delete the readResponse() method if it exists | ||
sed -i '/func (o \*NodeBootstrappingGetDefault) readResponse/,/^}/d' "$FILE" | ||
|
||
echo "readResponse() method deleted from $FILE if it existed. This is for a temporary fix that is in node_bootstrapping_get_responses_override.go." |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,7 @@ tools() { | |
go install github.com/rhysd/actionlint/cmd/[email protected] | ||
go install github.com/mattn/[email protected] | ||
go install github.com/google/go-containerregistry/cmd/[email protected] | ||
go install github.com/go-swagger/go-swagger/cmd/[email protected] | ||
|
||
if ! echo "$PATH" | grep -q "${GOPATH:-undefined}/bin\|$HOME/go/bin"; then | ||
echo "Go workspace's \"bin\" directory is not in PATH. Run 'export PATH=\"\$PATH:\${GOPATH:-\$HOME/go}/bin\"'." | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,4 +28,7 @@ const ( | |
NetworkDataplaneAzure = "azure" | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO, most of these should be in |
||
DefaultKubernetesMaxPods = 250 | ||
|
||
ProvisionModeAKSSelfContained = "aksselfcontained" | ||
ProvisionModeBootstrappingClient = "bootstrappingclient" | ||
) |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -103,10 +103,12 @@ func NewOperator(ctx context.Context, operator *operator.Operator) (context.Cont | |||
options.FromContext(ctx).ClusterEndpoint, | ||||
azConfig.TenantID, | ||||
azConfig.SubscriptionID, | ||||
azConfig.ResourceGroup, | ||||
azConfig.KubeletIdentityClientID, | ||||
azConfig.NodeResourceGroup, | ||||
azConfig.Location, | ||||
vnetGUID, | ||||
options.FromContext(ctx).ProvisionMode, | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need provision mode or would it be better to align on a NAP flag?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There could be more than one provisioning mode (including within NAP) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #527 (comment) yeah I responded down here with similar arguement. We could have multiple provisioning modes, also we could have mulitple "types" of karpenter self-hosted aks, managed aks, self-hosted azure, its arguable that even this boolean flag I am introducing in my PR would make more sense as a string with multiple options. |
||||
) | ||||
instanceTypeProvider := instancetype.NewProvider( | ||||
azConfig.Location, | ||||
|
@@ -129,6 +131,7 @@ func NewOperator(ctx context.Context, operator *operator.Operator) (context.Cont | |||
azConfig.Location, | ||||
azConfig.NodeResourceGroup, | ||||
azConfig.SubscriptionID, | ||||
options.FromContext(ctx).ProvisionMode, | ||||
) | ||||
|
||||
return ctx, &Operator{ | ||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -74,7 +74,9 @@ type Options struct { | |||||
SubnetID string // => VnetSubnetID to use (for nodes in Azure CNI Overlay and Azure CNI + pod subnet; for for nodes and pods in Azure CNI), unless overridden via AKSNodeClass | ||||||
setFlags map[string]bool | ||||||
|
||||||
NodeResourceGroup string | ||||||
NodeResourceGroup string | ||||||
ProvisionMode string | ||||||
NodeBootstrappingServerURL string | ||||||
} | ||||||
|
||||||
func (o *Options) AddFlags(fs *coreoptions.FlagSet) { | ||||||
|
@@ -90,6 +92,8 @@ func (o *Options) AddFlags(fs *coreoptions.FlagSet) { | |||||
fs.StringVar(&o.SubnetID, "vnet-subnet-id", env.WithDefaultString("VNET_SUBNET_ID", ""), "The default subnet ID to use for new nodes. This must be a valid ARM resource ID for subnet that does not overlap with the service CIDR or the pod CIDR") | ||||||
fs.Var(newNodeIdentitiesValue(env.WithDefaultString("NODE_IDENTITIES", ""), &o.NodeIdentities), "node-identities", "User assigned identities for nodes.") | ||||||
fs.StringVar(&o.NodeResourceGroup, "node-resource-group", env.WithDefaultString("AZURE_NODE_RESOURCE_GROUP", ""), "[REQUIRED] the resource group created and managed by AKS where the nodes live") | ||||||
fs.StringVar(&o.ProvisionMode, "provision-mode", env.WithDefaultString("PROVISION_MODE", consts.ProvisionModeAKSSelfContained), "[UNSUPPORTED] The provision mode for the cluster.") | ||||||
fs.StringVar(&o.NodeBootstrappingServerURL, "nodebootstrapping-server-url", env.WithDefaultString("NODEBOOTSTRAPPING_SERVER_URL", ""), "[UNSUPPORTED] The url for the node bootstrapping provider server.") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think most of our options have the full stop in their descriptions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, full stop seems correct. Just wanted it to be consistent, and looked at surrounding ones and not the full set. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed the other ones missing a full stop to have it. |
||||||
} | ||||||
|
||||||
func (o Options) GetAPIServerName() string { | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of thoughts:
//go:generate swagger generate client ...
etc.--keep-spec-order
?