Implements provider fields for subnets, securitygroups, launchtemplate, and instanceprofile. #681
Conversation
|
✔️ Deploy Preview for karpenter-docs-prod canceled. 🔨 Explore the source changes: 6064555 🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/61468105be7db1000890b55e |
|
I took a first pass on the WIP, looking good! |
ellistarn
force-pushed
the
ltsubnetsecgrp
branch
5 times, most recently
from
14a790d
to
c5db8d7
Compare
ellistarn
changed the title
ellistarn
force-pushed
the
ltsubnetsecgrp
branch
from
c5db8d7
to
43bc7f2
Compare
ellistarn
force-pushed
the
ltsubnetsecgrp
branch
from
43bc7f2
to
c2bed57
Compare
| errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s not a valid URL", c.Endpoint), "endpoint")) | ||
| for _, restricted := range RestrictedLabels { | ||
| if strings.Contains(key, restricted) { | ||
| errs = errs.Also(apis.ErrInvalidKeyName(key, "labels")) |
There was a problem hiding this comment.
Wondering if this error should be more descriptive? (Will it be obvious it's a restricted label?) Don't need to change, realize it was already like that.
There was a problem hiding this comment.
I'm relying on the knative concept for invalid key name. This error message looks like invalid key name "kubernetes.io/arch": spec.labels. Thoughts?
There was a problem hiding this comment.
it would be nice if it said "restricted" but that can wait until later
| if err != nil || !endpoint.IsAbs() || endpoint.Hostname() == "" { | ||
| errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s not a valid URL", c.Endpoint), "endpoint")) | ||
| for _, restricted := range RestrictedLabels { | ||
| if strings.Contains(key, restricted) { |
There was a problem hiding this comment.
Just curious what the intent is here? Just want to make sure we don't rather want something like startswith or something?
There was a problem hiding this comment.
Oh this is a really good catch! Moving to HasPrefix
ellistarn
force-pushed
the
ltsubnetsecgrp
branch
3 times, most recently
from
f9175f6
to
40b2e82
Compare
…, and instanceProfiles
ellistarn
force-pushed
the
ltsubnetsecgrp
branch
from
40b2e82
to
6064555
Compare
| errs = errs.Also(apis.ErrInvalidValue(fmt.Sprintf("%s not a valid URL", c.Endpoint), "endpoint")) | ||
| for _, restricted := range RestrictedLabels { | ||
| if strings.Contains(key, restricted) { | ||
| errs = errs.Also(apis.ErrInvalidKeyName(key, "labels")) |
There was a problem hiding this comment.
it would be nice if it said "restricted" but that can wait until later
1. Issue, if available:
#661
2. Description of changes:
Implements provider fields for subnets, securitygroups, launchtemplate, and instanceprofile.
Currently none of these are overridable at the pod level. This is done intentionally to maintain separation of concerns between cluster operators and application developers. This may change if we get feature requests for this in the future.
3. Does this change impact docs?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.