Add SVID hints on workload api client #220
Conversation
Signed-off-by: Daniel Feldman <dfeldman.mn@gmail.com>
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
guilhermocc
requested review from
amartinezfayo,
evan2645 and
azdagron
as code owners
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
There was a problem hiding this comment.
Thanks @guilhermocc! I have concerns, mostly around changing the exported functions without a clear understanding of the benefit we are getting.
v2/svid/common/optional/optional.go
Outdated
| package optional | ||
|
|
||
| // SVIDOptionals contains optional fields for SVIDs. | ||
| type SVIDOptionals struct { |
There was a problem hiding this comment.
I don't think it is likely that we'll gain any more "optional" fields on the SVIDs. I think I'd probably be more comfortable with just including the Hint field in each of the SVID structs.
There was a problem hiding this comment.
You are right, I did a bit of over eng here
v2/svid/x509svid/svid.go
Outdated
| @@ -47,7 +50,7 @@ func Load(certFile, keyFile string) (*SVID, error) { | |||
| // Parse parses the X509-SVID from PEM blocks containing certificate and key | |||
| // bytes. The certificate must be one or more PEM blocks with ASN.1 DER. The | |||
| // key must be a PEM block with PKCS#8 ASN.1 DER. | |||
| func Parse(certBytes, keyBytes []byte) (*SVID, error) { | |||
| func Parse(certBytes, keyBytes []byte, opts ...optional.SVIDOption) (*SVID, error) { | |||
There was a problem hiding this comment.
Now that 2.0 has shipped, I don't think we should be changing the signature of exported functions unless the benefit is very clear. In this case, I don't really have a good sense on how hints will be leveraged enough to understand if it's really a hardship for someone to call Parse and then set the hint on the returned SVID v.s. having a way to set the SVID hint while parsing. I'd probably lean towards not providing these until we fully understand how folks will want to interact with the hint.
There was a problem hiding this comment.
Makes sense, I will keep things simple here
v2/internal/test/ca.go
Outdated
| return certificateOption(func(c *x509.Certificate) { | ||
| c.SerialNumber = serial | ||
| }) | ||
| type SvidOption struct { |
There was a problem hiding this comment.
Where possible (everywhere expect protos), we should capitalize the full acronym in the name:
| type SvidOption struct { | |
| type SVIDOption struct { |
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
guilhermocc
force-pushed
the
add-spiffe-hints-workload-api-client
branch
from
706a3b4
to
926e51a
Compare
guilhermocc
requested review from
azdagron
and removed request for
evan2645 and
amartinezfayo
v2/svid/x509svid/svid.go
Outdated
| @@ -116,6 +120,11 @@ func (s *SVID) GetX509SVID() (*SVID, error) { | |||
| return s, nil | |||
| } | |||
|
|
|||
| // SetHint sets the hint for the SVID. | |||
| func (s *SVID) SetHint(hint string) { | |||
There was a problem hiding this comment.
What is the benefit of the setter?
v2/svid/jwtsvid/svid.go
Outdated
| @@ -85,6 +88,11 @@ func (svid *SVID) Marshal() string { | |||
| return svid.token | |||
| } | |||
|
|
|||
| // SetHint sets the hint for the JWT-SVID. | |||
| func (svid *SVID) SetHint(hint string) { | |||
There was a problem hiding this comment.
What is the benefit of the setter?
There was a problem hiding this comment.
Usually, I like to use setters for encapsulating eventual validation logic for setting the fields in the struct; especially in this case, in which this struct will and can be used by external applications. For now we don't have any validation here, and I'm not sure if we will have it in the future, so I would also be ok with not having this. What do you think?
There was a problem hiding this comment.
In this particular case, hint is an opaque string by spec, so there isn't really any validation and I don't anticipate needing it in the future.
Even so, we need to be very mindful of every exported type/function coming out of these packages since there is a compatibility guarantee that we make towards the stability of the interfaces. The bar should be very high towards adding new exported methods/types that we'd be obligated to maintain.
Signed-off-by: Guilherme Carvalho <guilhermocc@proton.me>
Add hint field to SVIDs retrieved by workload API client