-
Notifications
You must be signed in to change notification settings - Fork 326
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: access logs update for proxy-defaults CRD #1816
Conversation
09cd8f0
to
f073484
Compare
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.
🙏 excellent!
@@ -10,7 +10,7 @@ require ( | |||
github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 | |||
github.com/hashicorp/consul-k8s/control-plane/cni v0.0.0-20220831174802-b8af65262de8 | |||
github.com/hashicorp/consul-server-connection-manager v0.1.0 | |||
github.com/hashicorp/consul/api v1.18.0 | |||
github.com/hashicorp/consul/api v1.10.1-0.20221220195433-629878a6879c |
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.
we should look into this sometime in the future because it is awfully confusing that the API module version is 1.10.*
.
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.
Hard agree that it's messy. If I understand how the pseudo-version is created, it's because the last semver tag on Consul main
for api
module was api/1.10.0
. You can tell it's a holiday week because I made a shell snippet to verify:
git tag -l 'api/*' | while read tag
do
echo Tag is $tag
commit=$(git rev-list -n 1 $tag)
echo Commit is $commit
if git merge-base --is-ancestor $commit HEAD; then
echo "Yes"
else
echo "No"
fi
echo ""
done
All this to say, I'm not sure I have no idea how to fix it 😬 .
@@ -75,6 +75,8 @@ type ProxyDefaultsSpec struct { | |||
MeshGateway MeshGateway `json:"meshGateway,omitempty"` | |||
// Expose controls the default expose path configuration for Envoy. | |||
Expose Expose `json:"expose,omitempty"` | |||
// AccessLogs controls all envoy instances' access logging configuration. | |||
AccessLogs *AccessLogs `json:"accessLogs,omitempty"` | |||
} | |||
|
|||
func (in *ProxyDefaults) GetObjectMeta() metav1.ObjectMeta { |
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.
nit: public method should have comment (I know you didn't create this)
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.
Looks like the ConfigEntryResource
interface has a comment for this. I don't see comments in other implementations, so I'll probably pass on updating this round.
f073484
to
8142f57
Compare
Changes proposed in this PR:
proxy-defaults
, feat: add access logging API to proxy defaults consul#15780How I've tested this PR:
How I expect reviewers to test this PR:
Checklist: