-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Switch to using external ECR credential provider for k8s 1.27 #15342
Switch to using external ECR credential provider for k8s 1.27 #15342
Conversation
/hold for completion |
9fced04
to
3376292
Compare
nodeup/pkg/model/kubelet.go
Outdated
@@ -413,6 +434,56 @@ func (b *KubeletBuilder) usesContainerizedMounter() bool { | |||
} | |||
} | |||
|
|||
// addECRCP installs the ECR credential provider | |||
func (b *KubeletBuilder) addECCRCP(c *fi.NodeupModelBuilderContext) error { |
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.
Seems to be a typo.
func (b *KubeletBuilder) addECCRCP(c *fi.NodeupModelBuilderContext) error { | |
func (b *KubeletBuilder) addECRCP(c *fi.NodeupModelBuilderContext) error { |
pkg/apis/kops/cluster.go
Outdated
@@ -220,6 +220,9 @@ type AWSSpec struct { | |||
// Spotinst cloud-config specs | |||
SpotinstProduct *string `json:"spotinstProduct,omitempty"` | |||
SpotinstOrientation *string `json:"spotinstOrientation,omitempty"` | |||
|
|||
// BinaryLocation is the location of the AWS cloud provider binaries. | |||
BinaryLocation *string `json:"binaryLocation,omitempty"` |
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.
BinaryLocation
seems a little vague to me.
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.
Yeah I know. I hate it. But I couldn't find a good name for this.
FTR: the motivation behind this one is to use the PR/staging bucket in the AWS cloud provider e2e tests.
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.
Maybe BinariesLocation
, in case there will be more in the future?
if c.Cluster.UsesExternalECRCredentialsProvider() { | ||
binaryLocation := c.Cluster.Spec.CloudProvider.AWS.BinaryLocation | ||
if binaryLocation == nil { | ||
binaryLocation = fi.PtrTo("https://artifacts.k8s.io/binaries/cloud-provider-aws/v1.27.1") |
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.
Should v1.27.1
be something like AWS CCM version? Not sure if we have access to that here.
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.
Its a good catch. I was thinking of a helper method here where we feed in the K8s version and returns the cloud provider version. And move the case statement from CCM into that function. But I thought there wasn't a rush to do this right now.
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.
No rush.
3376292
to
d7ba611
Compare
/retest |
/hold cancel |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hakman The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
No description provided.