Feat integrate spiffe #1663
Feat integrate spiffe #1663
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: TessaIO The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
cncf-cla: yes
✅ Deploy Preview for kubernetes-sigs-nfd ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
TessaIO
force-pushed
the
feat-integrate-spiffe
branch
4 times, most recently
from
b60483c
to
2917fe1
Compare
|
/cc marquiz |
|
Let's close #1434 ;) |
k8s-ci-robot
added
the
needs-rebase
TessaIO
force-pushed
the
feat-integrate-spiffe
branch
2 times, most recently
from
c433995
to
5e5183d
Compare
k8s-ci-robot
removed
the
needs-rebase
| @@ -91,6 +91,8 @@ func main() { | |||
| klog.InfoS("-port is deprecated, will be removed in a future release along with the deprecated gRPC API") | |||
| case "verify-node-name": | |||
| klog.InfoS("-verify-node-name is deprecated, will be removed in a future release along with the deprecated gRPC API") | |||
| case "enable-spiffe": | |||
There was a problem hiding this comment.
@marquiz should we define this as a feature-gate???
There was a problem hiding this comment.
Probably not. I think this will never be enabled by default so feature gate feels superfluous (and unintuitive). It would serve as a unnecessary second-level gate (we'd want the enable/disable setting anyway), i.e. you'd need to specify -feature-gates spiffe=true -enable-spiffe
| "github.com/stretchr/testify/assert" | ||
| "sigs.k8s.io/node-feature-discovery/api/nfd/v1alpha1" |
There was a problem hiding this comment.
Is this file go imports checked?
There was a problem hiding this comment.
Does it contain any problem? My IDE does not complain about it.
k8s-ci-robot
added
the
needs-rebase
TessaIO
force-pushed
the
feat-integrate-spiffe
branch
from
5e5183d
to
4e5af99
Compare
k8s-ci-robot
removed
the
needs-rebase
TessaIO
force-pushed
the
feat-integrate-spiffe
branch
2 times, most recently
from
65e5b03
to
aee2686
Compare
k8s-ci-robot
added
the
needs-rebase
| @@ -91,6 +91,8 @@ func main() { | |||
| klog.InfoS("-port is deprecated, will be removed in a future release along with the deprecated gRPC API") | |||
| case "verify-node-name": | |||
| klog.InfoS("-verify-node-name is deprecated, will be removed in a future release along with the deprecated gRPC API") | |||
| case "enable-spiffe": | |||
There was a problem hiding this comment.
Probably not. I think this will never be enabled by default so feature gate feels superfluous (and unintuitive). It would serve as a unnecessary second-level gate (we'd want the enable/disable setting anyway), i.e. you'd need to specify -feature-gates spiffe=true -enable-spiffe
There was a problem hiding this comment.
Wouldn't we want to depend on/refer to some official Spire Helm chart for deploying spire instead of maintaining our own?
There was a problem hiding this comment.
yes for Helm charts that would be better. But I'm wondering how we're going to do that for Kustomize.
| @@ -85,6 +90,7 @@ type NFDConfig struct { | |||
| LeaderElection LeaderElectionConfig | |||
| NfdApiParallelism int | |||
| Klog klogutils.KlogConfigOpts | |||
| EnableSpiffe bool | |||
There was a problem hiding this comment.
Hmm, looking at this maybe it would be safest to only have this as a command line argument (i.e. NOT at as a dynamically configurable config file setting). Just to make it very clear if/when the setting is changed. WDYT?
Signed-off-by: TessaIO <[email protected]>
Signed-off-by: TessaIO <[email protected]>
TessaIO
force-pushed
the
feat-integrate-spiffe
branch
from
aee2686
to
520dd72
Compare
k8s-ci-robot
added
needs-rebase
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/milestone v0.17 |
|
@TessaIO: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1663 +/- ##
==========================================
- Coverage 39.85% 39.49% -0.37%
==========================================
Files 80 81 +1
Lines 6839 6996 +157
==========================================
+ Hits 2726 2763 +37
- Misses 3859 3968 +109
- Partials 254 265 +11
|
|
/milestone v0.18 |
Resolves #1186
TODO: